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

if command is ran with a compose file, apply the compose model #9375

Merged
merged 4 commits into from Aug 3, 2022

Conversation

ndeloof
Copy link
Contributor

@ndeloof ndeloof commented Apr 11, 2022

What I did
pass project to start/restart/pause/unpause if user ran command with a compose.yaml file, not just project-name
backend will then apply model (active profiles) and not consider all resources with project label

Related issue
see #9286

Fixes #9705
Fixes #9671

(not mandatory) A picture of a cute animal, if possible in relation with what you did
image

@ndeloof ndeloof force-pushed the project_or_name branch 8 times, most recently from 901a2f6 to 97eb94e Compare April 11, 2022 13:30
@@ -62,7 +61,7 @@ func downCommand(p *projectOptions, backend api.Service) *cobra.Command {
ValidArgsFunction: noCompletion(),
}
flags := downCmd.Flags()
removeOrphans := utils.StringToBool(os.Getenv("COMPOSE_REMOVE_ORPHANS "))
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 wonder this ever worked before?

@laurazard
Copy link
Member

I fixed the merge conflicts and added some of the tests I'd already written for the other PR, this one covers the other use case and applies the same logic to other commands 🥳

Copy link
Contributor

@milas milas left a comment

Choose a reason for hiding this comment

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

🥳

@laurazard laurazard requested a review from milas August 2, 2022 19:09
@laurazard
Copy link
Member

Ahh! Need to fix some tests 😅

ndeloof and others added 3 commits August 2, 2022 22:33
…ust project name

Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Signed-off-by: Laura Brehm <laurabrehm@hey.com>
Signed-off-by: Laura Brehm <laurabrehm@hey.com>
@laurazard
Copy link
Member

laurazard commented Aug 3, 2022

While looking into why this was failing when applying a similar logic to compose ps I noticed some inconsistencies regarding how the -p flag is handled vs the COMPOSE_PROJECT_NAME env var.

The reason why some tests were failing was that we are running compose ps with the COMPOSE_PROJECT_NAME env var set, but without a compose.yaml in the directory. Using projectOptions.toProjectName(...), this worked because before attempting to load the project, it checks for the env var and doesn't attempt to load the project otherwise. However, the introduction of projectOptions.projectOrName(...) causes some issues here because it doesn't attempt to check the env var, and instead always loads the project unless the passed projectOptions struct already has a name set. The inconsistencies here are due to the fact that the projectOptions.ProjectName field is set when running compose -p [project-name], but not when running COMPOSE_PROJECT_NAME=[project-name] compose.

I think we should handle these in the same way, probably in the parent compose command, and set that value regardless of where the project name comes from. This would also make some other things more consistent, ie compose -p [name] down works even if there is no compose.yaml in the directory but COMPOSE_PROJECT_NAME=[name] compose down fails.

I made some changes to projectOrName() in this PR to fallback to the previous behavior so the tests can pass, but I think some of this logic should be cleaned up and moved up for more consistency.

@ndeloof
Copy link
Contributor Author

ndeloof commented Aug 3, 2022

compose -p [name] down works even if there is no compose.yaml in the directory but COMPOSE_PROJECT_NAME=[name] compose down fails

Why would you like the latter to fail? IMHO once COMPOSE_PROJECT_NAME is set, all compose command should be able to run based on existing resources, same as passing -p flag. But maybe I'm missing some corner cases...

@laurazard
Copy link
Member

laurazard commented Aug 3, 2022

I worded that weirdly 😅 don’t want it to fail, but with the changes before my last commit it was failing. This was due to projectOrName not checking the env var.

@ndeloof
Copy link
Contributor Author

ndeloof commented Aug 3, 2022

LGTM with @laurazard additions

Copy link
Contributor

@glours glours left a comment

Choose a reason for hiding this comment

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

LGTM 👍

pkg/e2e/fixtures/start-stop/other.yaml Outdated Show resolved Hide resolved
Signed-off-by: Laura Brehm <laurabrehm@hey.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants