This repository has been archived by the owner on Mar 24, 2023. It is now read-only.
Improvements and fixes to ship watch
#432
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dexhorthy
commented
Aug 21, 2018
| @@ -52,107 +37,48 @@ func (s *Ship) stateFileExists(ctx context.Context) bool { | |||
| return !noExistingState | |||
| } | |||
|
|
|||
| func (s *Ship) Update(ctx context.Context) error { | |||
| debug := level.Debug(log.With(s.Logger, "method", "update")) | |||
| func (s *Ship) Init(ctx context.Context) error { | |||
There was a problem hiding this comment.
the diff on this file looks weird because I moved update/watch out of here. I recommend side-by-side
dexhorthy
commented
Aug 21, 2018
| } | ||
| } | ||
|
|
||
| func (s *Ship) Update(ctx context.Context) error { |
There was a problem hiding this comment.
no changes here, just moved
dexhorthy
commented
Aug 21, 2018
| return errors.New(`No current SHA found at ` + s.Viper.GetString("state-file") + `, please run "ship init"`) | ||
| } | ||
|
|
||
| debug.Log("event", "fetch latest chart") |
dexhorthy
commented
Aug 21, 2018
| defaultSpec *api.Spec, | ||
| ) (*api.Release, error) { | ||
| debug := log.With(level.Debug(r.Logger), "method", "resolveChart") | ||
|
|
||
| metadata, err := r.resolveMetadata(context.Background(), upstream, localPath) | ||
| err := util.BackupIfPresent(r.FS, destPath, debug, r.ui) |
There was a problem hiding this comment.
this backup/rename logic got moved up out of determineApplicationType
laverya
reviewed
Aug 21, 2018
| @@ -60,6 +60,7 @@ var _ = Describe("ship app", func() { | |||
| var testMetadata TestMetadata | |||
|
|
|||
| BeforeEach(func() { | |||
| os.Setenv("NO_OS_EXIT", "1") | |||
There was a problem hiding this comment.
Could we store this and restore it after the integration tests?
There was a problem hiding this comment.
Not sure why we'd need to do that? I don't think it persists into the parent process?
There was a problem hiding this comment.
Hmm. Might be a good thing to check when running with ginkgo
What I Did ------------ Improve and fix some things with `ship watch`, Resolve #428 ``` $ bin/ship watch --interval 30s github.com/dexhorthy/sysdigcloud-kubernetes/sysdigcloud has an update available ``` How I Did it ------------ - apptype.Inspector no longer moves the cloned assets up from .ship/tmp into "base" or "k8s" - resolver.ResolveRelease handles moving chart up from .ship/tmp into "base" or "chart" - remove defer os.RemoveAll in apptype.Inspector, is in `.ship/tmp` now, so it will get cleaned up eventually - more/better debug logging in `ship watch` - bind viper flags in `cli/watch.go` so `viper.GetDuration("interval")` works in watch command - added some UI output for ship watch Other stuff: - move watch and update into their own files - clean .ship/tmp on startup in case its hanging around How to verify it ------------ `ship init ...; ship watch --interval 30s` Description for the Changelog ------------ Add configurable interval for `ship watch` via `--interval` flag 
laverya
reviewed
Aug 21, 2018
| // if there's a Chart.yaml, assume its a chart | ||
| isChart, err := r.fs.Exists(path.Join(savePath, "Chart.yaml")) | ||
| if err != nil { | ||
| return "", "", errors.Wrap(err, "check for Chart.yaml") | ||
| } | ||
| debug.Log("event", "isChart.check", "isChart", isChart) |
There was a problem hiding this comment.
Should this be before the check?
There was a problem hiding this comment.
i think its for writing the result of the check, hence the isChart field
laverya
approved these changes
Aug 21, 2018
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What I Did
Improve and fix some things with
ship watch, Resolve #428How I Did it
.ship/tmpnow, so it will get cleaned up eventuallyship watchcli/watch.gosoviper.GetDuration("interval")works in watch commandOther stuff:
How to verify it
ship init ...; ship watch --interval 30sDescription for the Changelog
Add configurable interval for
ship watchvia--intervalflagWhat I Did
How I Did it
How to verify it
Description for the Changelog
Picture of a Boat (not required but encouraged)