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(cli): allow specifying integration run sources exclusively via config file #5930

Merged
merged 2 commits into from
Nov 13, 2024

Conversation

fsrv-xyz
Copy link
Contributor

@fsrv-xyz fsrv-xyz commented Nov 7, 2024

Hey there!

Thank you for doing such awesome work with that project.

While using the kamel tool, I tried to exclusively configure my deployment via the kamel-config.yaml file. Due to there is a sources array in that config file, I think specifying the sources there should be enough to run an integration rather than needing to define at least one source at kamel run [file to run].

My patch skips the error if no source file is provided via arguments and at least one source is provided in the configuration file. Otherwise, there will be an error if the sources are only provided in config file.

Copy link
Contributor

@squakez squakez 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 having a look at the problem. I am not entirely sure this change is going to affect other behaviors. I'll have a look more carefully later and let you know if we can merge this. In the while, you may report the problem opening an issue, so, we can track it.

@fsrv-xyz
Copy link
Contributor Author

fsrv-xyz commented Nov 8, 2024

I opened an issue at #5933 to keep track of this. Thank you for checking.

Copy link
Contributor

@squakez squakez 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 the contribution. However there is a small flaw here when we'd be running an empty execution which has not anything either in the kamel-config.yaml.

kamel run -o yaml --name test would result in an Integration with no sources, resulting in a potential disruption on the operator. You may want to enhance the PR checking also the presence of any valid configuration on the config profile. However, mind that the config profile was deprecated in the latest 2.5.0 release, so, likely dropped from 2.7 version onward.

@fsrv-xyz
Copy link
Contributor Author

@squakez Thank you for your review. The behavior you described was related to an error on my end where an empty sources list passed the check. I fixed that in b041f17.

I also checked your mention of the "config profile" part.
While I am not 100% sure what you mean by that (especially its impact on the sources), I guess it is the deprecation you introduced in #5871. While browsing the docs for that, I could not identify a way of manipulating the source file selection with IntegrationProfiles. Please enlighten me if I have overlooked something.

@squakez squakez linked an issue Nov 13, 2024 that may be closed by this pull request
@squakez squakez merged commit 90ada2a into apache:main Nov 13, 2024
10 checks passed
@squakez
Copy link
Contributor

squakez commented Nov 13, 2024

Thanks for the contribution!

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.

Do not fail if sources are exclusively passed via configuration file.
2 participants