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.

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

Makefile will now process all files in "values" directory #30

Closed
wants to merge 4 commits into from

Conversation

myspotontheweb
Copy link

@myspotontheweb myspotontheweb commented Feb 2, 2019

These changes make the automation more hands off

Testing

Create two dummy files to represent 2 additional kops clusters

echo "" > values/demo1.k8s.local.yml
echo "" > values/demo2.k8s.local.yml

Demonstrate the commands generated by Make (Note the "--name" parameters being passed so that multiple clusters can be updated)

$ make --dry-run clean update
rm -rf dist/clusters/cluster-exampleenv.yml dist/clusters/demo1.k8s.local.yml dist/clusters/demo2.k8s.local.yml
kops toolbox template --fail-on-missing \
                      --format-yaml \
                      --template src/eu-west-1/templates \
                      --snippets src/eu-west-1/snippets \
                      --values values/cluster-exampleenv.yml \
                      > dist/clusters/cluster-exampleenv.yml
kops replace --force -f dist/clusters/cluster-exampleenv.yml --name cluster-exampleenv
kops update cluster --name cluster-exampleenv
kops toolbox template --fail-on-missing \
                      --format-yaml \
                      --template src/eu-west-1/templates \
                      --snippets src/eu-west-1/snippets \
                      --values values/demo1.k8s.local.yml \
                      > dist/clusters/demo1.k8s.local.yml
kops replace --force -f dist/clusters/demo1.k8s.local.yml --name demo1.k8s.local
kops update cluster --name demo1.k8s.local
kops toolbox template --fail-on-missing \
                      --format-yaml \
                      --template src/eu-west-1/templates \
                      --snippets src/eu-west-1/snippets \
                      --values values/demo2.k8s.local.yml \
                      > dist/clusters/demo2.k8s.local.yml
kops replace --force -f dist/clusters/demo2.k8s.local.yml --name demo2.k8s.local
kops update cluster --name demo2.k8s.local

Note:

  • The additional "clean" target is useful to force an update of all clusters.

@ghost ghost self-assigned this Feb 3, 2019
@ghost ghost added the enhancement New feature or request label Feb 3, 2019
@ghost
Copy link

ghost commented Feb 3, 2019

Thank you for this contribution. I have few questions.

  • Why did you decide to add clean target?
    It is really difficult to imagine situation, when removing dist/clusters would make sense.

  • Why did you define variables (very good idea) and you didn't use them in commands?

  • Last but not least. In our environments, there are circumstances, besides these I am the only one in charge ( 😃 ), where some of SREs have access to few clusters and some to others.
    Your target assumes, you have access to everything. Very good for CI/CD pipelines. Not good for manual changes. And even at CI/CD it would make sense to use sts upgrading specific cluster.
    Have a look at: Add assumeRole support #31.
    Describe workflow, please.


#
# Rules
#
dist/clusters/%.yml: values/%.yml
Copy link

Choose a reason for hiding this comment

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

Use variables.

Copy link
Author

@myspotontheweb myspotontheweb Feb 5, 2019

Choose a reason for hiding this comment

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

I used the built in automatic variables: $@ and $<, described here:

https://www.gnu.org/software/make/manual/html_node/Automatic-Variables.html

$@ represents the target name and $< represents the source (or prerequiste) name in the recipe command.

The real target is actually a so-called phony one "update"

update: $(CLUSTER_FILES)

The list of prerequites is matched against available rules.

Copy link

@ghost ghost Feb 5, 2019

Choose a reason for hiding this comment

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

  • I know these variables. You use them as target definition.

  • What I mean, is that you defined variables on the top of Makefile, that actually, define properly all necessary files (values and clusters), and in the end you do not use these as your target.

How I would see that:

(FILES_CLUSTERS): $(FILES_VALUES)

dist/clusters/%.yml: values/%.yml
kops toolbox template --fail-on-missing \
--format-yaml \
--template src/eu-west-1/templates \
--snippets src/eu-west-1/snippets \
--values $< \
> $@
kops replace --force -f $@
kops update cluster
kops replace --force -f $@ --name $(patsubst dist/clusters/%.yml,%,$@)
Copy link

Choose a reason for hiding this comment

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

Use variables, the same below.


#
# Rules
Copy link

Choose a reason for hiding this comment

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

How rules differ from targets?

Copy link
Author

Choose a reason for hiding this comment

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

The rule merely describes how the target is created.

Copy link

@ghost ghost Feb 5, 2019

Choose a reason for hiding this comment

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

target : prereq
    recipe

This is the rule, how target is created and when it needs to be updated.

Ah! See. Correct me, if I am wrong.
You say: It is intended for the user to use targets only. Rules are internal definitions..

@@ -44,7 +47,7 @@ kops toolbox template --fail-on-missing \

* Generate a cluster from template:
```
kops create -f clusters/cluster-exampleenv.yml
kops create -f dist/clusters/cluster-exampleenv.yml
Copy link

Choose a reason for hiding this comment

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

Good catch! 👍

@myspotontheweb
Copy link
Author

Remaining questions:

clean target

I always include one as a way to recover from cases of partial command execution, or to simply force a regeneration of all files, for example

make clean update

Never have to use it, I include in all my Makefiles

SREs with limited access

Note sure the point you're making about sts and assumed roles.

We have a simpler setup with kops clusters managed from a single shared S3 bucket, in each cloud account. We purposely don't share buckets between accounts, in order to isolate prod and nonprod infrastructure.

@ghost
Copy link

ghost commented Feb 5, 2019

  • Clean target. Approve.

  • SREs with limited access. At the moment: Approve.
    This is a kind of kops limitation as well, we will handle at @lintops.

@ghost
Copy link

ghost commented Feb 9, 2019

@myspotontheweb A week passed since you have opened a pull-request.
We would be happy to support you to make it completed and merged.

@ghost ghost closed this Feb 23, 2019
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant