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

Feature: Added force-fetch functionality (#858) #862

Merged
merged 6 commits into from
Oct 30, 2022

Conversation

MatteoVoges
Copy link
Contributor

@MatteoVoges MatteoVoges commented Oct 4, 2022

Fixes issue: #858

Proposed Changes

  • Added a option to have a force_fetch-property in your external dependencies.
    If force_fetch is set to true, this dependency will always get fetched and overwrite existing files (force-fetch).

  • Renamed --force-flag to --force-fetch, so you have to run

    • $ kapitan compile --fetch to fetch (existing files won't get overwritten) or
    • $ kapitan compile --force-fetch to fetch the dependencies and overwrite existing files.

If the property is false or not given, the default-behavior of the fetch-functionality doesn't change.
Also the behavior of --fetch and (now:)--force-fetch doesn't change either.

NOTE: Using --force will still work with its default behavior, but is now a deprecated flag and will be removed soon.

Documentation and testing

Updated flag-name in the docs and in the tests.

@MatteoVoges
Copy link
Contributor Author

Hi @ramaro @ademariag ,

I applied the changes we discussed, so the PR is ready to review.

But there is one more thing I noticed:
I don't like the way the code for the fetch-functionality is structured. There are multiple if-statements checking the same, and its hard to follow and unterstand the code in general. I would suggest something like this:

[...]
# getting the flag values
if fetch:
    # fetch inventories
    # fetch dependencies
else:
    # fetch targets with fetch_always tag
[...]

So with that we would have one if-statement, which indicates if we are fetching or not.

Should I dump my suggestion into this PR or wait until this PR is merged and then create a new one?

@ramaro
Copy link
Member

ramaro commented Oct 24, 2022

@MatteoVoges thanks for getting this done!

Should I dump my suggestion into this PR or wait until this PR is merged and then create a new one?
I think doing this in a new PR makes sense to keep things simpler!

@ramaro
Copy link
Member

ramaro commented Oct 24, 2022

This lgtm @MatteoVoges ! Would you mind updating the doc changes and title to force_fetch instead of fetch_always? We can merge straight after that,

@MatteoVoges MatteoVoges changed the title Feature: Added fetch_always functionality Feature: Added force-fetch functionality (#858) Oct 25, 2022
@MatteoVoges
Copy link
Contributor Author

Would you mind updating the doc changes and title to force_fetch instead of fetch_always?

Hey @ramaro,
I updated the title and the PR description. What else should be updated?

@ramaro
Copy link
Member

ramaro commented Oct 25, 2022

@MatteoVoges there's some fetch_always usage in docs and code. Particularly because during our discussion, we've agreed to rename to force_fetch and this is not reflected for example in valid_target_obj(). Unless I'm missing something here 🤔

@MatteoVoges
Copy link
Contributor Author

@ramaro , you're right. I updated it now. Please let me know if there is still something to do here.

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.

None yet

2 participants