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

Implements stack listing and change detection #5

Merged
merged 1 commit into from
Nov 2, 2021
Merged

Conversation

i4ki
Copy link
Contributor

@i4ki i4ki commented Oct 14, 2021

The change detection mimics the internal make/bash scripts but is not the optimal solution. We will improve it after the MVP validation.

soerenmartius
soerenmartius previously approved these changes Oct 14, 2021
Copy link
Contributor

@soerenmartius soerenmartius left a comment

Choose a reason for hiding this comment

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

lgtm from what I can tell! Really great job @i4ki ! You rock!

@soerenmartius
Copy link
Contributor

@thiesen maybe you can review it from a more go point of view :)

@i4ki i4ki changed the title WIP: Implements stack listing and change detection Implements stack listing and change detection Oct 15, 2021
@i4ki i4ki requested review from zwopir and thiesen October 15, 2021 03:46
@@ -1,2 +1,53 @@
# terrastack
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@soerenmartius @mariux

I wrote here my understanding of terraform stacks but I'm not sure if it's accurate. Please let me know if makes sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

will check and give you detailed feedback

Copy link
Contributor

Choose a reason for hiding this comment

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

will check..
sorry for the delay

Copy link
Contributor

Choose a reason for hiding this comment

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

Just read through it now. Thanks for the comprehensive description @i4ki it reads really well. One of the major upsides of a stack that should be be forgotten is that it decreases the execution time of terraform by a lot also. I feel we should include this in the why also - @mariux could you also provide your input here? Thanks

Copy link
Contributor

Choose a reason for hiding this comment

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

time is another good point... this also comes with coat reduction as CI runs are payed by time so significantly reducing this also reduces number of build minutes needed..

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 improved by adding the reduced time benefit.

init.go Outdated Show resolved Hide resolved
thiesen
thiesen previously approved these changes Oct 15, 2021
Copy link

@thiesen thiesen left a comment

Choose a reason for hiding this comment

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

man, this is a beautiful PR!

terrastack.go Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Comment on lines +36 to +84
1. The default branch (commonly main) has the production (applied) code.
2. Before planning and apply, the changes must be committed in a feature/bugfix
branch.
3. The IaC project must use [non
fast-forwarded](https://git-scm.com/docs/git-merge#_fast_forward_merge) merge
commits. (the default in github and bitbucket).
Copy link
Contributor

Choose a reason for hiding this comment

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

yes... the mostimportant to note here:

each merge to main needs to be easily detectable e.g. by using get ref like HEAD^ or HEAD~ we need to be able to point to the last merge, or any number of merges e.g. HEAD~4

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I improve here somehow? Maybe we should avoid being too technical about git here, since using merge commits already makes this implicit.

README.md Outdated Show resolved Hide resolved
mariux
mariux previously approved these changes Oct 15, 2021
Copy link
Contributor

@mariux mariux left a comment

Choose a reason for hiding this comment

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

lgtm .... cannot run go-lang inmy head yet.... ;) so cannot judge the code... added some coment in the docs though.... great work afaict

@i4ki
Copy link
Contributor Author

i4ki commented Oct 15, 2021

.... cannot run go-lang inmy head yet.... ;)

@mariux I'm working in the cli now =) Next PR will be easy to test it working.

@i4ki i4ki dismissed stale reviews from mariux and thiesen via b7e0990 October 16, 2021 08:37
Copy link

@zwopir zwopir left a comment

Choose a reason for hiding this comment

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

good work! I left some comments.

Consider using the if _, err := funcReturnsWhateverAndErr(); err != nil { ... if you only check for error. Also in some cases multiple nested if conditions can be ANDed

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
git/git.go Show resolved Hide resolved
git/git.go Outdated Show resolved Hide resolved
git/git.go Show resolved Hide resolved
git/git.go Outdated Show resolved Hide resolved
git/git.go Outdated Show resolved Hide resolved
git/git.go Show resolved Hide resolved
git/git.go Show resolved Hide resolved
@@ -1,2 +1,53 @@
# terrastack
Copy link
Contributor

Choose a reason for hiding this comment

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

Just read through it now. Thanks for the comprehensive description @i4ki it reads really well. One of the major upsides of a stack that should be be forgotten is that it decreases the execution time of terraform by a lot also. I feel we should include this in the why also - @mariux could you also provide your input here? Thanks

soerenmartius
soerenmartius previously approved these changes Oct 26, 2021
Copy link
Contributor

@soerenmartius soerenmartius left a comment

Choose a reason for hiding this comment

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

lgtm!

@i4ki
Copy link
Contributor Author

i4ki commented Nov 2, 2021

@thiesen @zwopir @soerenmartius @mariux

I just squashed the commits. Can you guys approve for merging?

@soerenmartius soerenmartius self-requested a review November 2, 2021 10:58
Copy link
Contributor

@soerenmartius soerenmartius left a comment

Choose a reason for hiding this comment

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

lgtm!

@i4ki i4ki merged commit ad05f1f into main Nov 2, 2021
@i4ki i4ki deleted the changed-stacks branch November 2, 2021 11:00
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.

5 participants