-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
apps sc & wc: improve dry-run and deploy scripts #431
Conversation
42e1130
to
1fdb78d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly looking for explanations. Otherwise this looks fine!
1fdb78d
to
1740d56
Compare
003800c
to
dacf6f7
Compare
We had a meeting with the outcome: The changes could be activated by using a command flag |
193f254
to
34d6e2f
Compare
Now i have presented a start of a solution. Not the most beautiful but it works. "apply-sync" and "dry-run-sync". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to instead of introducing a new command, having a flag instread? Like
$ bin/ck8s apply --sync
I guess something like getops would be something but that doesn't support long arguments (only single letter). If it's not feasable I'm okey with this approach as well.
I would also like to rename the dry-run-sync
to dry-run-kubectl
since it has nothing to do with sync.
There's also some formatting issues I have commented on
1a5bfc9
to
c1d537e
Compare
So, now i have done a lot of changes.
|
c1d537e
to
3d0e2f4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This really starts to look good! There are still some things left though.
e2b5dc8
to
f21e149
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some minor details
f21e149
to
157b988
Compare
Done! Looks a lot smoother now indeed! 😄 👍 |
157b988
to
63e8cbd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only have some formatting issues now but otherwise this looks great!
63e8cbd
to
0945230
Compare
Done! 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just the last few things 😅
0945230
to
614f212
Compare
614f212
to
6d257c3
Compare
Please resolve some of the comments that's been fixed. A bit cumbersome to overview the pr ;) |
6d257c3
to
a182ce7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, well done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Nice :)
a182ce7
to
233757e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Great work
ec021a6
to
63039ac
Compare
What this PR does / why we need it:
The dry-run.sh and deploy scripts we currently use will not check against the actual state of the cluster. We are now adding commando flags to enable the option of using sync if a user want to.
Which issue this PR fixes
fixes #382
Message to reviewers
Im not sure if this is the right way to do it. This was the closest i could figure out. Feel free to point me out in the right direction if needed.
Checklist: