StackFile support #167

taraspos · 2017-11-27T23:04:28Z

Hey,

I know you guys are busy with preparation for ReInvent, but I prepared small PR with stack-file support. No need to merge it now, but I will appreciate some review and feedback.

One thing that is missing is the CLI usage/description for stack-file, since, AFAIK all the descriptions generated automatically from appropriate AWS documentation, but stack-file not on the docs.

BTW, in this PR many vendor files are removed, I think it is because you forgot to run dep prune

implements Issue #145

run "dep prune"

simcap · 2017-11-28T09:43:53Z

Good job @trane9991 . It seems you quickly got a grasp of our new way of wrapping around AWS (i.e. cloud commands), although there are still some rough edges and things to improve on our side.

Thanks for the work so far! We do not have a section on how to contribute to awless, so I will give you random hints that can help (you might already know):

the generation logic takes place in the gen folder (need a lot of cleaning actually in term of making the code more readable). You should not need to touch this part.
generated files will have by convention the gen_ prefix. They should not be edited obviously.
at the root of awless we run go generate generate.go when modifying cloud commands (as you did) or the models in general. This call should be idempotent.
documentation that is not automatically generated can be added manually in aws/doc/paramsdoc.go (next to aws/doc/gen_paramsdoc.go)

To validate the project we then have:

at the root run go test ./... -v that you can do whenever
./smoke_test/smoke_test.sh and a mastondonte template but those ones cost money (create/revert stuff on the cloud) so you don't have to do it.

More importantly, we now have acceptance tests for each cloud commands. In your case, this would be in the file aws/acceptance/stack_test.go. Could you add the corresponding sub tests? They should be feasible.

Also I am wondering if the dep prune should be done in another PR so this one could be merged without risking a unexpected effect, and we could revert commits independently of the other.

This reverts commit 4b73b22.

add Makefile add yaml vendor

Had to sort tags/parameters for the tests predictability

taraspos · 2017-11-28T23:00:51Z

Hey,
added tests, descriptions, reverted dep prune so we do it in separate PR, also added small Makefile to generate, test and build code.

simcap · 2017-11-29T08:54:54Z

Makefile

@@ -0,0 +1,11 @@
+test:
+	@echo Running tests
+	@go test $$(go list ./... | grep -v /vendor/)


Since we expect Golang 1.9.x to be used, excluding vendor should not be necessary anymore. So a go test ./... here should suffice and will be quicker.

Right, I will remove it

simcap · 2017-11-29T09:12:36Z

aws/spec/stack.go

+// https://github.com/wallix/awless/issues/145
+// http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/continuous-delivery-codepipeline-cfn-artifacts.html
+func (cmd *UpdateStack) BeforeRun(ctx map[string]interface{}) error {
+	// don't do anything if StackFile not provided


Wondering if we can we avoid duplication and do an extract method on the BeforeRun of Update and Create .... if the end result is not too ugly and makes sense ;) ?

Make complete sense, I thought about it too, just didn't have time yet, to play with it a bit more.

simcap · 2017-11-29T09:22:31Z

acceptance/aws/stack_test.go

+	newFilePath := strings.Join([]string{file.Name(), ext}, ".")
+	if err = os.Rename(file.Name(), newFilePath); err != nil {
+		panic(err)
+	}


Shall we instead of a extra rename use just https://golang.org/pkg/io/ioutil/#TempDir, Join ioutil.WriteFile?

Sure, make sense.

simcap · 2017-11-29T09:33:05Z

acceptance/aws/stack_test.go

+}
+
+func generateTmpFileWithExt(content, ext string) (*os.File, string, func()) {
+	file, err := ioutil.TempFile("", "awless-at-tmpfile")


I would then put the signature of the method with a varargs like generateTmpFileWithExt(content, filename ... string) for an optional filename (i.e. with any extension if needed) so that the caller control the name if needed and makes clear the intent

simcap · 2017-11-29T09:40:09Z

Did a quick review. Thanks for the work.

Before merging if you don't mind it would be good to:

make sure you tried error cases where you have empty/malformed/rubbish input in the slurp stack file. Ensuring that it fails fast; CLI errors are focused and tell what is going wrong.
if you could show us (copy/paste in this issue) a real life example of what you do on the CLI, basically the multi liners (or maybe the one liner) you run ;) .. to get a sense of the final usage

taraspos · 2017-11-29T09:45:54Z

Negative tests is a very good idea. I tried it locally, but it would be nice to also add them to the tests.

@simcap do you mean the example of final usage with the stack-file or without it?

simcap · 2017-11-29T09:58:44Z

Yeap, a full one liner or (multi liner) capturing the different way you do manage stack creation/ and update ... You might have that in your bash history (removing sensitive param if needed of course)

simcap · 2017-11-29T10:00:11Z

Anyway, I am just curious on how you manage the CI (i guess) pipeline with AWS and awless (even if I have some ideas). There might be stuff we could still improve or take into account. If you write an article about it let us know.

taraspos · 2017-11-29T10:02:51Z

Yep, I'm using it with the CI. I'm planning to prepare slides about the process somewhere later in December, I will share it with you once it is ready.

Also, I will share usage example on the Issue comment, once finish with this PR.

simcap · 2017-11-29T10:04:18Z

Thanks. Examples are mostly to put in the changelog along the new feature.

taraspos · 2017-11-30T21:02:42Z

Hey@simcap,
now it should be ready for merge.

simcap · 2017-12-01T10:58:13Z

Thanks. If you confirm that you have tested live error cases (empty, rubbish, unexisting files) and they have a clear and helpful output, I will merge it today.

add a bit better error messages

taraspos · 2017-12-01T19:17:04Z

@simcap
now it is ready, made errors a bit more user-friendly.

taraspos · 2017-12-04T12:59:28Z

@simcap thanks 🙇

taraspos added 6 commits November 27, 2017 00:34

initial implementation of "stack-file" support

57f999c

separate code into different functions

26630d7

add couple more nil checks

7a121d5

add more comments

9fc609c

fix round to seconds

60ad6c0

add yaml vendor

4b73b22

run "dep prune"

taraspos added 4 commits November 28, 2017 19:08

Revert "add yaml vendor"

ff15df4

This reverts commit 4b73b22.

add stack-file param description

e3ea544

add Makefile add yaml vendor

implement some "stack-file" tests

f67829c

Had to sort tags/parameters for the tests predictability

run tests before build

da10e78

simcap reviewed Nov 29, 2017

View reviewed changes

taraspos added 5 commits November 29, 2017 23:31

use TmpDir to generate named temp file

0efc0f5

move StackFile processing into separate func

f44e174

Merge branch 'master' of https://github.com/wallix/awless

e3d6175

do not return any policy body if stackfile is not used

4035caa

better output for stack events tail

0366802

revert change with time.duration.Round() since it works with go1.9

21ce7fd

add one more error check,

a58161c

add a bit better error messages

simcap merged commit d92e86a into wallix:master Dec 4, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

StackFile support #167

StackFile support #167

taraspos commented Nov 27, 2017 •

edited

Loading

simcap commented Nov 28, 2017

taraspos commented Nov 28, 2017

simcap Nov 29, 2017

taraspos Nov 29, 2017

simcap Nov 29, 2017

taraspos Nov 29, 2017

simcap Nov 29, 2017

taraspos Nov 29, 2017

simcap Nov 29, 2017

simcap commented Nov 29, 2017

taraspos commented Nov 29, 2017

simcap commented Nov 29, 2017

simcap commented Nov 29, 2017

taraspos commented Nov 29, 2017

simcap commented Nov 29, 2017

taraspos commented Nov 30, 2017

simcap commented Dec 1, 2017

taraspos commented Dec 1, 2017

taraspos commented Dec 4, 2017

StackFile support #167

StackFile support #167

Conversation

taraspos commented Nov 27, 2017 • edited Loading

simcap commented Nov 28, 2017

taraspos commented Nov 28, 2017

simcap Nov 29, 2017

Choose a reason for hiding this comment

taraspos Nov 29, 2017

Choose a reason for hiding this comment

simcap Nov 29, 2017

Choose a reason for hiding this comment

taraspos Nov 29, 2017

Choose a reason for hiding this comment

simcap Nov 29, 2017

Choose a reason for hiding this comment

taraspos Nov 29, 2017

Choose a reason for hiding this comment

simcap Nov 29, 2017

Choose a reason for hiding this comment

simcap commented Nov 29, 2017

taraspos commented Nov 29, 2017

simcap commented Nov 29, 2017

simcap commented Nov 29, 2017

taraspos commented Nov 29, 2017

simcap commented Nov 29, 2017

taraspos commented Nov 30, 2017

simcap commented Dec 1, 2017

taraspos commented Dec 1, 2017

taraspos commented Dec 4, 2017

taraspos commented Nov 27, 2017 •

edited

Loading