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

refactor(fetch): rework force-fetch process #986

Closed
wants to merge 3 commits into from

Conversation

MatteoVoges
Copy link
Contributor

@MatteoVoges MatteoVoges commented Apr 6, 2023

Reference #862 #858

Proposed Changes

  • Reworked force-fetch functionality with property in force_fetch: true in kapitan dependencies
  • Fixed logical error where targets were skipped, if one dependency has no entry force_fetch
  • Now only those dependencies with force_fetch: true get fetched and not the entire targets dependencies

There's nothing to do regarding tests and documentation, because it exists already.


Contributed by

@MatteoVoges MatteoVoges added enhancement enhancement to an existing feature bugfix fixing a bug labels Apr 20, 2023
@ademariag ademariag requested a review from ramaro August 23, 2023 09:26
Comment on lines +148 to +154
if not dep_entry.get("force_fetch", False):
# if no fetch is needed remove the dependency and don't fetch it
target["dependencies"].remove(dep_entry)

# fetch dependencies with force_fetch set to true
if target.get("dependencies", []):
fetch_objs.append(target)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

idk why I reversed the logic here: so if it hasn't the flag, then we remove it...
Should I fix that?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it makes sense to have a way to force on the command line "not to force" in case you are running from a place with no external connectivity

So i'd probably leave it in as it is, we can change it later if it doesn't make sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely, but I think it would work the same if we say, that: if force_fetch: true --> force_fetch, instead of removing it from the list, if it don't have force_fetch: true

Copy link
Contributor Author

@MatteoVoges MatteoVoges Aug 29, 2023

Choose a reason for hiding this comment

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

a way to force on the command line "not to force"

So you want a --dont-force-fetch flag?

Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps --offline-mode so it can be a generic way to turn off all attempts to download things?

@ramaro what to you think?

I am thinking of the case where you run it in places with no connectivity or you just don't want to change anything. It would suck if in those cases it kept failing.

Alternatively, we could handle it with an additional:

fail_on_error: false

@github-actions github-actions bot added the Stale label Nov 9, 2023
@github-actions github-actions bot removed the Stale label May 11, 2024
@github-actions github-actions bot added the Stale label Jul 10, 2024
@ademariag ademariag closed this Sep 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix fixing a bug enhancement enhancement to an existing feature Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants