Skip to content

Support setting namespace for every deployer #852

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

Merged
merged 4 commits into from
Aug 7, 2018

Conversation

dgageot
Copy link
Contributor

@dgageot dgageot commented Jul 24, 2018

Support setting namespace for every deployer via --namespace flag or SKAFFOLD_NAMESPACE env variable.

Also :

  • Used this feature in the integration tests and
  • Simplified the code of the integration tests

@dgageot dgageot added the wip label Jul 24, 2018
@dgageot dgageot force-pushed the namespace branch 3 times, most recently from f41a3bb to 21d3dd8 Compare July 25, 2018 00:09
@dgageot dgageot removed the wip label Jul 25, 2018
@dgageot dgageot force-pushed the namespace branch 3 times, most recently from 7ef1c2c to b82c2f0 Compare July 26, 2018 18:59
@dgageot dgageot force-pushed the namespace branch 2 times, most recently from 7c71ad4 to 7f25aa5 Compare July 27, 2018 22:37
@balopat balopat self-assigned this Aug 3, 2018
@@ -107,3 +109,22 @@ func buildManifests(kustomization string) (io.Reader, error) {
}
return bytes.NewReader(out), nil
}

// TODO(dgageot): this code is already in KubectlDeployer
Copy link
Contributor

Choose a reason for hiding this comment

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

why wouldn't we just add the namespace parameter to the kubectl method which we would leave as a package level utility? I'm not sure I follow why both the kustomizeDeployer and the KubeCtlDeployer need kubectl command. What is your plan here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't really have a plan. The thing is that both deployer deploy with kubectl. I wanted to embed KubeCtlDeployer into kustomizeDeployer but I'm not sure it's really better than a little duplication.
Another plan would be to extract this function and the state it requires into a shared package

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I extracted the kubectl CLI code to a separate package to make easier to share between both deployers.

ns, err := client.CoreV1().Namespaces().Create(&v1.Namespace{
ObjectMeta: meta_v1.ObjectMeta{
Name: namespaceName,
Namespace: namespaceName,
GenerateName: "skaffold",
Copy link
Contributor

Choose a reason for hiding this comment

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

nice!

},
{
description: "helm example",
args: []string{"run"},
deployments: []testObject{
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 for the yagni cleanup.

@dgageot dgageot force-pushed the namespace branch 3 times, most recently from 996c4b1 to 4f79a65 Compare August 6, 2018 07:17
dgageot added 4 commits August 6, 2018 19:55
via flag or env variable.

Signed-off-by: David Gageot <[email protected]>
Signed-off-by: David Gageot <[email protected]>
Signed-off-by: David Gageot <[email protected]>
Copy link
Contributor

@balopat balopat left a comment

Choose a reason for hiding this comment

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

LGTM

@dgageot dgageot merged commit f91a57d into GoogleContainerTools:master Aug 7, 2018
@dgageot dgageot deleted the namespace branch December 28, 2018 07:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants