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

Always use the default workspace directory when building commands context #1620

Merged

Conversation

giuli007
Copy link
Contributor

@giuli007 giuli007 commented Jun 7, 2021

Fixes #1612

When pre_workflow_hooks is being used to generate atlantis.yaml and projects are set with specific workspaces different than default, a different directory is created for each project-workspace pair, but all those that are not on the default workspace will not have an atlantis.yaml file at their root.

As described in some details in #1612 (comment) this can lead to situations where atlantis tries to build commands without finding the atlantis.yaml even though it has correctly being generated in the default workspace.

The most obvious error is

building command for dir "<path to project>": cannot specify a project name unless an atlantis.yaml file exists to configure projects

when trying to atlantis apply.

But there are also more subtle errors with atlantis plan -d <projectdir> -w <workspacename> or atlantis apply -d <projectdir> -w <workspacename> where they don't throw errors but implicitly use the DefaultProjCfg which is not what the end user would expect.

Note that this is a bit inconsistent because when autoplan, atlantis plan or atlantis apply -p are used it all seem to work fine.

I believe the issue lies in what repoDir parameter is passed to buildProjectCommandCtx

From a bit of digging the invocations which cause issues are:

On the contrary the following scenarios don't cause the issues mentioned above:

  • buildPlanAllCommands(for atlantis plan and autoplan) does not use the above function and always builds all commands using the default workspace and therefore doesn't cause any issue
  • atlantis plan -p and atlantis apply -p invoke the buildProjectPlanCommand and buildProjectApplyCommand which both default to use the default workspace and thus pass a directory with atlantis.yaml to buildProjectCommandCtx and also don't cause any issue

This change ensures that when atlantis is building the projects commands for either:

  • a project-specific plan, triggered via (atlantis plan -d foo -w bar)
  • a project-specific apply, triggered vi (atlantis apply -d foo -w bar)
  • or applies for all the plans it can find (atlantis apply)

it uses the default workspace to find the atlantis.yaml config.

First time I dig into the Atlantis codebase so please let me know if the reasoning above makes sense.

@giuli007 giuli007 requested a review from a team as a code owner June 7, 2021 18:24
@giuli007 giuli007 force-pushed the always_use_default_ws_config_which_fixes_1612 branch 3 times, most recently from fa6c02d to 9989ebe Compare June 7, 2021 20:15
Copy link
Contributor

@msarvar msarvar left a comment

Choose a reason for hiding this comment

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

PR looks good overall, good job catching the issue!
Would be good to add some tests around the change. Mainly to make sure that the project context is not losing the workspace when it is passed into DefaultProjectCommandRunner

@giuli007
Copy link
Contributor Author

giuli007 commented Jun 17, 2021

@msarvar thanks for the comment.

Would be good to add some tests around the change. Mainly to make sure that the project context is not losing the workspace when it is passed into DefaultProjectCommandRunner

Initially I was also concerned about this as I thought that changing the repoDir passed to buildProjectCommandCtx could affect the resulting ProjectCommandContextes which are eventually used by DefaultProjectCommandRunner to run the commands.
However I think this is not the case: the directory that is used when building the command context is not part of ProjectCommandContext and it doesn't get reused when running the commands that instead re-calculate/clone it again based on the workspace name (e.g. in doPlan).
In light of this I think the only concern might be if someone in the future changes this code so that ProjectCommandContext starts to carry around a reference to repoDir that was used to create it.

I have looked at the existing tests for inspiration but I could not come up with an easy way to test the scenario you suggest in a reasonable time.
I think it is a bit tricky because it involves different components and thus is difficult to unit-test; moreover the existing harnesses for the e2e tests don't provide a good ground to reproduce the mentioned use-case because they entirely mock out the repository cloning so they are not exercising the code that creates different directories for different workspaces.

Do you have any more specific suggestions about how to create some tests?

As a general comment I think the project would benefit from more comprehensive tests that simulate:

  • atlantis.yaml generation from pre_workflow_hooks
  • how using non-default workspaces result in different directories

But this goes a bit beyond the scope of this PR.

@msarvar
Copy link
Contributor

msarvar commented Jun 22, 2021

@giuli007 what you say makes sense, and it is a bit tricky to test it in a meaningful way. I'm ok with merging this as is, PR needs to pass the checks through, can you take a look and try to the status check?

Copy link
Contributor

@msarvar msarvar left a comment

Choose a reason for hiding this comment

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

thanks for fixing the issue!

@msarvar
Copy link
Contributor

msarvar commented Jun 22, 2021

@giuli007 I think the PR needs to be rebased, and that should fix the failing status check.

…text

When pre_workflow_hooks is being used to generate atlantis.yaml
and projects are set with specific workspaces different than "default",
a different directory is created for each project-workspace pair,
but all those that are not on the "default" workspace will
not have an atlantis.yaml file at their root.

This change ensures that when atlantis is building the projects commands
for either:
- a project-specific plan, triggered via (atlantis plan -d foo -w bar)
- a project-specific apply, triggered vi (atlantis apply -d foo -w bar)
- or applies for all the plans it can find (atlantis apply)
it uses the default workspace to find the atlantis.yaml config.
@giuli007 giuli007 force-pushed the always_use_default_ws_config_which_fixes_1612 branch from 9989ebe to ad22408 Compare June 24, 2021 16:55
@giuli007
Copy link
Contributor Author

@msarvar thank you taking the time to review and approve. A rebase fixed the issue so it should be good to go

@msarvar msarvar merged commit 528ee85 into runatlantis:master Jun 24, 2021
msarvar referenced this pull request in lyft/atlantis Sep 27, 2021
…text (#1620)

When pre_workflow_hooks is being used to generate atlantis.yaml
and projects are set with specific workspaces different than "default",
a different directory is created for each project-workspace pair,
but all those that are not on the "default" workspace will
not have an atlantis.yaml file at their root.

This change ensures that when atlantis is building the projects commands
for either:
- a project-specific plan, triggered via (atlantis plan -d foo -w bar)
- a project-specific apply, triggered vi (atlantis apply -d foo -w bar)
- or applies for all the plans it can find (atlantis apply)
it uses the default workspace to find the atlantis.yaml config.

Co-authored-by: giuli007 <[email protected]>
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.

atlantis doesnt use default atlantis.yaml unless a project name is specified with preworkflow hooks
2 participants