Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Move logic out of the Configuration container #34

Merged
merged 1 commit into from
Aug 10, 2020
Merged

Move logic out of the Configuration container #34

merged 1 commit into from
Aug 10, 2020

Conversation

ccremer
Copy link
Contributor

@ccremer ccremer commented Jul 1, 2020 •
edited
Loading

It shouldn't be necessary to figure out the namespace from kubeconfig if we just pass --version flag

Other changes:

  • We only want -n flag and the ns/image args to be used to specify the namespace, to support future registries that don't rely on kubeconfig's configured namespace
  • The default namespace is not used as a fallback anymore

Closes: #33

@ccremer ccremer requested a review from zugao July 1, 2020 13:39
bittner
bittner reviewed Jul 2, 2020
View reviewed changes
cfg/types.go Outdated Show resolved Hide resolved
bittner
bittner reviewed Jul 2, 2020
View reviewed changes
Copy link
Contributor

@bittner bittner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a minor thing on the namespace default that needs clarification. LGTM otherwise.

zugao
zugao reviewed Jul 6, 2020
View reviewed changes
Copy link
Contributor

@zugao zugao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything seems ok but I would wait a bit with merging this branch to master. I already did a lot of refactoring to add the gomock framework

@ccremer
Copy link
Contributor Author

ccremer commented Jul 6, 2020

I would recommend the other way around: Merge this and then you can rebase your PR on the latest master branch. Because there have been other changes introduced lately.

@zugao
Copy link
Contributor

zugao commented Jul 6, 2020

I would recommend the other way around: Merge this and then you can rebase your PR on the latest master branch. Because there have been other changes introduced lately.

You want me buried in rebase? :D

@ccremer ccremer force-pushed the namespace branch 4 times, most recently from f0c0758 to 5ccc01e Compare August 5, 2020 14:25
@ccremer
Move logic out of the Configuration container
4213b7b 
- It shouldn't be necessary to figure out the namespace if we just pass --version flag
- other small improvements
- The version will now be printed as Info at startup, to prevent confusion in pipelines.

Signed-off-by: Chris <github.account@chrigel.net>
@ccremer ccremer merged commit a728ac2 into master Aug 10, 2020
@ccremer ccremer deleted the namespace branch August 10, 2020 11:09
@ccremer ccremer mentioned this pull request Aug 10, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bittner bittner bittner left review comments

@zugao zugao zugao left review comments

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Seiso should not attempt to load kubeconfig when printing the version
3 participants
@ccremer @zugao @bittner