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

Ensure only the Git cache directory from the local worker config is used #5438

Merged

Conversation

Martchus
Copy link
Contributor

@Martchus Martchus commented Jan 25, 2024

  • Prevent users from cluttering directories when accidentally specifying a wrong GIT_CACHE_DIR
  • Avoid the Git cache directory configured on a production worker to be used when cloning a job to the local instance (where that directory might not even exists or another cache directory is wanted)
  • See https://progress.opensuse.org/issues/154240

Note that I haven't extended tests because we already have a test for the changed line and we likely don't need to test the same behavior for all the relevant keys.

* Prevent users from cluttering directories when accidentally specifying a
  wrong `GIT_CACHE_DIR`
* Avoid the Git cache directory configured on a production worker to be
  used when cloning a job to the local instance (where that directory might
  not even exists or another cache directory is wanted)
* See https://progress.opensuse.org/issues/154240
Copy link

codecov bot commented Jan 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (99100fe) 98.37% compared to head (6085a1a) 98.37%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #5438   +/-   ##
=======================================
  Coverage   98.37%   98.37%           
=======================================
  Files         389      389           
  Lines       37744    37744           
=======================================
  Hits        37132    37132           
  Misses        612      612           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@perlpunk perlpunk left a comment

Choose a reason for hiding this comment

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

If the setting isn't used at all, why is it there?
Can we first discuss this please?

@Martchus
Copy link
Contributor Author

It might be there because a user created a job explicitly specifying it but in production we still want to use what's configured by us. (You have mentioned yourself that users might mistakes which can be dangerous.)

I'm actually not sure anymore whether openqa-clone-job would actually pull that setting in if it is just in vars.json and not in the openQA database. However, I think it makes sense to just teat it as a "local setting" like we do for other settings already regardless of that.

Copy link
Member

@kalikiana kalikiana left a comment

Choose a reason for hiding this comment

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

As discussed the change makes sense.

@mergify mergify bot merged commit 6ca6857 into os-autoinst:master Jan 25, 2024
41 checks passed
@Martchus Martchus deleted the git-cache-dir-only-from-worker-cfg branch January 25, 2024 13:16
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

4 participants