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: no logs when there is no source #643

Merged
merged 3 commits into from
Apr 14, 2022

Conversation

lemeurherve
Copy link
Member

@lemeurherve lemeurherve commented Apr 13, 2022

Fixes #617
Supersedes #640

Don't output anything about source when there isn't any sources section.

Test

To test this pull request, you can run the following commands:

./dist/updatecli diff --values examples/values.yaml --config examples/updatecli.d/updatecli-no-sources.yaml

With https://github.com/updatecli/updatecli/blob/main/examples/updatecli.d/updatecli.yaml without its sources block:

Before:
#############################
# UPDATECLI-NO-SOURCES.YAML #
#############################


SOURCES
=======


CONDITIONS:
===========
WARNING: The directive 'scm' for the condition["dockerFile"] is now deprecated. Please use the new top level scms syntax
WARNING: The directive 'scm' for the target["dockerFile"] is now deprecated. Please use the new top level scms syntax

[...]
After:
#############################
# UPDATECLI-NO-SOURCES.YAML #
#############################


CONDITIONS:
===========
WARNING: The directive 'scm' for the condition["dockerFile"] is now deprecated. Please use the new top level scms syntax
WARNING: The directive 'scm' for the target["dockerFile"] is now deprecated. Please use the new top level scms syntax

[...]

Additional Information

Tradeoff

Potential improvement

@olblak
Copy link
Member

olblak commented Apr 13, 2022

can you elaborate why you closed the Pullrequest #640 in favor of this one? I think I preferred the other approach even though I couldn't tested it yet

@lemeurherve
Copy link
Member Author

lemeurherve commented Apr 13, 2022

Because In my previous PR these ifs (L13 of each stage file like https://github.com/lemeurherve/updatecli/blob/cb37b0bf1efcf8bcee09f9e26f55f3bcf7245a00/pkg/core/pipeline/conditions.go#L13) were useless since the calls to Run<Stage> are already inside if len(p.<stages>) > 0 blocks for conditions, targets and pullrequests stages.

Thus, I prefered doing the same for sources instead of adding conditional instructions never reached for conditions & targets.

@olblak
Copy link
Member

olblak commented Apr 14, 2022

Because In my previous PR these ifs (L13 of each stage file like https://github.com/lemeurherve/updatecli/blob/cb37b0bf1efcf8bcee09f9e26f55f3bcf7245a00/pkg/core/pipeline/conditions.go#L13) were useless since the calls to Run<Stage> are already inside if len(p.<stages>) > 0 blocks for conditions, targets and pullrequests stages.

Thus, I prefered doing the same for sources instead of adding conditional instructions never reached for conditions & targets.

It makes a lot of sense, thank you for looking into this

@olblak olblak enabled auto-merge (squash) April 14, 2022 06:37
@olblak olblak merged commit d7702fa into updatecli:main Apr 14, 2022
@dduportal dduportal added the bug Something isn't working label Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Empty sources are displayed while it should not
4 participants