-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
feat: Add project depends on functionality #3292
feat: Add project depends on functionality #3292
Conversation
@@ -62,6 +69,8 @@ type ProjectContext struct { | |||
PullReqStatus models.PullReqStatus | |||
// CurrentProjectPlanStatus is the status of the current project prior to this command. | |||
ProjectPlanStatus models.ProjectPlanStatus | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you add a comment here of what this is doing?
will these work by running |
Thanks for the contribution @Luay-Sol |
Actually, this feature is more for the case where the user tries to apply to a specific project/workspace. What you have described already exists in Atlantis and is called
So with this, if the user just comments |
Amazing that is what I was expecting and I can see how useful this will be. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work. Thank you for your contribution. Please address nitpick comments. I envision we can deprecate execution_order_group
after this. It's a bit ambiguous. I feel like overloading depends_on
is a better place. If you need the projects executed in a particular order, that implies dependency.
I can see needing only "ordering" vs a "hard fail" like dependencies. How difficult would it be to add a key somewhat like:
version: 3
projects:
- dir: .
name: project-2
depends_on:
softFail: false
projects: [project-1]
Thoughts?
server/core/config/raw/project.go
Outdated
@@ -31,6 +31,7 @@ type Project struct { | |||
PlanRequirements []string `yaml:"plan_requirements,omitempty"` | |||
ApplyRequirements []string `yaml:"apply_requirements,omitempty"` | |||
ImportRequirements []string `yaml:"import_requirements,omitempty"` | |||
Dependencies []string `yaml:"depends_on,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: Can we follow the annotation style and rename the variable to DependsOn
server/core/config/raw/project.go
Outdated
@@ -72,12 +73,17 @@ func (p Project) Validate() error { | |||
return errors.Wrapf(err, "parsing: %s", branch) | |||
} | |||
|
|||
Dependencies := func(value interface{}) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: Can we follow the annotation style and rename the variable to DependsOn
server/core/config/raw/project.go
Outdated
return validation.ValidateStruct(&p, | ||
validation.Field(&p.Dir, validation.Required, validation.By(hasDotDot)), | ||
validation.Field(&p.PlanRequirements, validation.By(validPlanReq)), | ||
validation.Field(&p.ApplyRequirements, validation.By(validApplyReq)), | ||
validation.Field(&p.ImportRequirements, validation.By(validImportReq)), | ||
validation.Field(&p.TerraformVersion, validation.By(VersionValidator)), | ||
validation.Field(&p.Dependencies, validation.By(Dependencies)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: Can we follow the annotation style and rename the variable to DependsOn
server/core/config/raw/project.go
Outdated
@@ -121,6 +127,8 @@ func (p Project) ToValid() valid.Project { | |||
|
|||
v.Name = p.Name | |||
|
|||
v.Dependencies = p.Dependencies |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: Can we follow the annotation style and rename the variable to DependsOn
@@ -88,6 +85,7 @@ type MergedProjectCfg struct { | |||
ImportRequirements []string | |||
Workflow Workflow | |||
AllowedWorkflows []string | |||
Dependencies []string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename to DependsOn
server/core/config/valid/repo_cfg.go
Outdated
@@ -129,6 +129,7 @@ type Project struct { | |||
PlanRequirements []string | |||
ApplyRequirements []string | |||
ImportRequirements []string | |||
Dependencies []string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename to DependsOn
// Atlantis uses this information to valid the apply | ||
// orders and to warn the user if they're applying a project that | ||
// depends on other projects. | ||
Dependencies []string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename to DependsOn
}, | ||
}, | ||
wantFailure: "Can't apply your project unless you apply its dependencies: [project2]", | ||
wantErr: assert.NoError, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: Tests are good so far, can we include some more scenarios that include 3 projects to make sure Atlantis doesn't continue past the 2nd project?
Project 1: success
Project 2: fail
Project 3: skipped
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Luay-Sol - re: the bug you were struggling with, is there a test case in here that ensures no regressions against that bug? (in case I change the depends_on code)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, so the bug was related to missing
&& project.Status != models.PlannedNoChangesPlanStatus
to break the dependency chain, and I see above you already added a test case for this 🎉
@@ -65,6 +67,20 @@ func (a *DefaultCommandRequirementHandler) ValidateApplyProject(repoDir string, | |||
return "", nil | |||
} | |||
|
|||
func (a *DefaultCommandRequirementHandler) ValidateProjectDependencies(ctx command.ProjectContext) (failure string, err error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dependencies
makes sense here for the function name. I know I mentioned renaming some of the variables to DependsOn
. I'm not going to nit pick on every single one, just use good judgment on which ones make sense to change.
Thanks for your feedback @GenPage. For your first point about deprecating For your second point, It is not complicated to add such a key. However, I would like to better understand the use case. I am trying to figure out when a softfail will be used. If it is just to prevent failure, maybe not define any dependencies to the project. Thanks. |
@nitrocode While it is true there's no issue with having both, I see merit points to merging the two. At the moment it is confusing to have 2 features that fundamentally try to do the same thing, i.e create an execution order, but each one handles a different use case, The way I envision this is if we get the depend_on to do both, it will be much simpler for the user, to define it only once. |
ah ok, ty for the clarification. |
|
@Luay-Sol Sounds good.
The idea is you would still get ordering, but the project doesn't stop the continuation of dependency execution. If we were to deprecate e.g. "Hey, I want to execute in this order but I don't care if this project fails to apply, I want to rest of the dependencies to run" We can address that in a separate PR, where-as this PR just adds |
@Luay-Sol Any update on addressing some of the code comments? |
@jamengual @GenPage @vincentgna can I get final review on this? Anything else needed from me or I can do to get it out? Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Luay-Sol Excellent work, thank you for taking the time to contribute this.
@Luay-Sol Are you able to resolve the merge conflicts locally? We cannot resolve them via the web UI. Can you rebase from the Atlantis main branch please? |
Hi, just to check my understanding — in earlier discussions on this PR, the idea that this replaces |
@glasser yes, you understood it correctly. Here's an example of how I use it:
|
* feat: implemented the code for the depends on functionality Co-authored-by: Vincent <[email protected]>
* feat: implemented the code for the depends on functionality Co-authored-by: Vincent <[email protected]>
* feat: implemented the code for the depends on functionality Co-authored-by: Vincent <[email protected]>
what
depends_on
to the project config in theatlantis.yaml
file.why
Using the
depends_on
functionality will allow Atlantis to handle cases requiring one project to be applied prior to the other. In other words, allow promotions between projects/environments (dev -> staging -> production)tests
references
apply_requirements
#391, but not exactly. The implementation followed in this PR does not depend on the apply requirement key.