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

fix: --dependency flag is respected by dependency resolving #2708

Merged
merged 2 commits into from
Sep 5, 2023
Merged

fix: --dependency flag is respected by dependency resolving #2708

merged 2 commits into from
Sep 5, 2023

Conversation

Andrioden
Copy link
Contributor

@Andrioden Andrioden commented Sep 4, 2023

What issue type does this pull request address? (keep at least one, remove the others)
/kind bugfix

What does this pull request do? Which issues does it resolve? (use resolves #<issue_number> if possible)
Partly solves #2641

Please provide a short message that should be published in the DevSpace release notes
Fixed an issue where DevSpace flag --dependency was not respected by the git repo recursive resolver

What else do we need to know?

I have not ran any unit tests or e2e tests as your contribution guide and bash scripts dont work on windows.

I have tested manually by building a devspace.exe file and then tested these 3 cases standing in a repo with a complex dependency hierarchy:

1 - Normal C:\Projects\devspace-deps-fix\devspace.exe deploy --debug
-> Download all repos and deploy all

2 - Filter away all C:\Projects\devspace-deps-fix\devspace.exe deploy --dependency "-" --debug
-> Download no repos, and deploy only self

3 - Filter to one dependency C:\Projects\devspace-deps-fix\devspace.exe deploy --dependency "hn-oppsett" --debug
-> Download dependency, and deploy it and self

@netlify
Copy link

netlify bot commented Sep 4, 2023

Deploy Preview for devspace-docs canceled.

Built without sensitive environment variables

Name Link
🔨 Latest commit b81076b
🔍 Latest deploy log https://app.netlify.com/sites/devspace-docs/deploys/64f7609cacec8400083cbcc3

@Andrioden Andrioden changed the title fix: --dependency flag is respected by git pulling fix: --dependency flag is respected by dependency resolving Sep 4, 2023
@@ -39,7 +40,7 @@ func NewManagerWithParser(ctx devspacecontext.Context, configOptions *loader.Con

type ResolveOptions struct {
SkipDependencies []string
Dependencies []string
Dependencies []string // Same as DependencyOptions.Only, either should be renamed
Copy link
Contributor Author

@Andrioden Andrioden Sep 4, 2023

Choose a reason for hiding this comment

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

The devspace deploy --dependency flag should be renamed --only, but thats beyond this pr and not backwards compatible. So i added this comment for clarity.

Resolve when comment is read.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think the flags will be renamed. As you mentioned, there's backwards compatibility to consider, but also changing the global flags may make them ambiguous. The BuildOptions() function handles mapping the global flag to the more specific run_dependency_pipelines function options.

Otherwise the changes look good! Thank you so much!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment removed, need workflow approval (i think? not that familiar with these GitHub gates)

Copy link
Collaborator

@lizardruss lizardruss left a comment

Choose a reason for hiding this comment

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

Consider removing a comment.

LGTM!

@@ -39,7 +40,7 @@ func NewManagerWithParser(ctx devspacecontext.Context, configOptions *loader.Con

type ResolveOptions struct {
SkipDependencies []string
Dependencies []string
Dependencies []string // Same as DependencyOptions.Only, either should be renamed
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think the flags will be renamed. As you mentioned, there's backwards compatibility to consider, but also changing the global flags may make them ambiguous. The BuildOptions() function handles mapping the global flag to the more specific run_dependency_pipelines function options.

Otherwise the changes look good! Thank you so much!

@lizardruss lizardruss merged commit c477eaf into devspace-sh:main Sep 5, 2023
18 checks passed
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.

2 participants