https://cloudposse.com logo
Join Slack
Powered by
<https://github.com/cloudposse/atmos/pull/2467> C...
# atmos-dev
s
https://github.com/cloudposse/atmos/pull/2467 Concurrent execution is ready for first review. It is working for plan/apply/destroy, I'm stuck on how to run it in interactive mode 🤔 I can create TUI for that where end user will need to go through each component in a stack to review and approve/decline, or I can skip it for now and instead do the CI part where auto-approve is enabled @Andriy Knysh (Cloud Posse) @Erik Osterman (Cloud Posse)
e
Awesome!! I will review today
I think we can defer the TUI; terraform can already prompt. Yes, in ci.enabled auto approve should be too
Thoughts on marking them ready for review?
s
yep, sorry G was down today, doing it
fixed
e
@Andriy Knysh (Cloud Posse) LGTM https://github.com/cloudposse/atmos/pull/2464
👀 1
Any comments?
@shirkevich overall these are looking really good
Minor nitpicks
Loving the stacked PRs
s
looks like that I fixed all the nitpicks? 🤔 how should we proceed further? will we wait until all the PRs will be ready or will merge them one by one on rediness?
e
Let's do this: we are planning a release today/tomorrow. Let's let it roll out, and then plan for this in 1.221.0
👍 1
We cut 1.220.0 lastnight
So we can start to merge your PRs
cc @Andriy Knysh (Cloud Posse)
I think I approved the first couple; might be worth @Andriy Knysh (Cloud Posse) taking a look
s
I've tried to address all the comments by coderabbitai and rebased all the PRs on each other again after each in the chain was accepted. sorry they all was force pushed several times due to that
e
Yea, that's the tradeoff with stacked prs
s
it will be good if you would try to run it against some real projects with "tf plan" to check the baseline and test corner cases that might sleep in synthetic testing. latest binary is working against my own tf modules with x2 speedup in concurrent execution
e
Also, see if you can get all PRs to 80%+
this is at 74%
s
addressed, fixed test/doc coverage, ci failures, flags, retriggered coderabbitai again
🙌 1
e
Awesome! I will review again tomorrow; let's try to get them merged.
s
@Erik Osterman (Cloud Posse) please ping me if you need smth from my side to proceed with DAG
e
@Andriy Knysh (Cloud Posse) ✅ LGTM your turn https://github.com/cloudposse/atmos/pull/2464
@Andriy Knysh (Cloud Posse) ✅ LGTM your turn https://github.com/cloudposse/atmos/pull/2465
@shirkevich what are your thoughts on changing this:
Copy code
--hide-no-changes                    Hide Terraform plan logs for components with no changes. Implies --log-
so instead it's 
--hide=no-changes
👍 1
That allows more things to be hidden without exploding args
similar to what we did with 
--skip
@Andriy Knysh (Cloud Posse) ✅ LGTM your turn https://github.com/cloudposse/atmos/pull/2474
👀 1
merged 1
@shirkevich are you able to squeeze out some more test coverage on #2519
other than that, LGTM
2 minor nits, and ✅ from @Andriy Knysh (Cloud Posse)
s
gotcha, fixing flag and test coverage
👍 1
Copy code
Done.

#2468 is pushed at 9051b69391c1040234c00beb1cf7743db9022318:

Replaced --hide-no-changes with extensible --hide.
--hide=no-changes now drives the existing no-change suppression behavior.
Added validation for unsupported hide values.
Updated help snapshots and tests.
#2519 is rebased on #2468 and pushed at 1ed70140af7a05d54fdbbac465d032dc213aded7:

Added scheduler coverage tests for affected graph selection and failure-mode helper paths.
pkg/scheduler/adapters now reports 82.6% statement coverage locally.
e
I will re-review in the morning
🙌 1
https://github.com/cloudposse/atmos/pull/2468 ✅ LGTM @Andriy Knysh (Cloud Posse) for final review
merged 1
https://github.com/cloudposse/atmos/pull/2519 ✅ LGTM @Andriy Knysh (Cloud Posse) for final review
merged 1
@shirkevich I think then with those merged, it's time to test everything in main and once validated we'll cut a release next week
s
@Erik Osterman (Cloud Posse) https://github.com/cloudposse/atmos/pull/2466 are we waiting now for final confirmation by team? I need to do CI testing and possibly have another PR with refines summary output, but I guess we can ship it separately after DAG will land
e
✅ LGTM
@Andriy Knysh (Cloud Posse) ☝️
Yes, I wouldnt' be surprised if there's one more follow up after all merged
Also, make sure then docs are up to date
and the blog post for announcing the feature
s
updated docs and added blog post to same PR
e
Are we able to squeeze out the last bit?
I think we're good to merge as soon as that's done
cc @Andriy Knysh (Cloud Posse)
I think we want to cut a release today or tomorrow
So let's try and get this in!
2 Views
Open in Slack
PreviousNext
Powered by