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

[JENKINS-69135] Add a "Versions to include" field to the Global Library Cache feature #16

Merged

Conversation

AnishaGharat
Copy link
Contributor

@AnishaGharat AnishaGharat commented Aug 16, 2022

You can now specify versions to be included for caching. This allows only those specific versions to be cached. This feature can be very useful when there are many versions and you want only one or more of them to be cached. It has the same working mechanism as that of the "Versions to exclude". One thing to be noted is that "Versions to exclude" will always take precedence over "Versions to include" for similar substrings mentioned in both at the same time.

This is pertaining to the Jenkins feature request: JENKINS-69135

This PR pertains to an elaborate documentation of the cache retrieval mechanism with the updated feature that current PR caters to.

Another important feature as per the PR: #4, to delete only a specific version from cache, was tested in collaboration with "Versions to include". The branch for the same: Versions to include + delete specific version.

Bonus Points: The cache deletion method for a specific version can be invoked remotely such that:
final cacheControl = new LibraryCachingConfiguration.DescriptorImpl();
cacheControl.doClearCache(libraryName, branchName, false);

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

@AnishaGharat
Copy link
Contributor Author

I will be further making changes to handle the scenario if the versions to include is null or " ", but the cache is enabled.

@AnishaGharat
Copy link
Contributor Author

I have updated the changes mentioned in the previous comment in the latest commit.
I have updated the documentation for the plugin that can viewed in the PR : jenkins-infra/jenkins.io#5375

@@ -493,7 +493,7 @@ public void parallelBuildsDontInterfereWithExpiredCache() throws Throwable {
new SCMSourceRetriever(new GitSCMSource(null, sampleRepo.toString(), "", "*", "", true)));
config.setDefaultVersion("master");
config.setImplicit(true);
config.setCachingConfiguration(new LibraryCachingConfiguration(30, null));
config.setCachingConfiguration(new LibraryCachingConfiguration(30, null,null));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
config.setCachingConfiguration(new LibraryCachingConfiguration(30, null,null));
config.setCachingConfiguration(new LibraryCachingConfiguration(30, null, null));

Copy link
Contributor

Choose a reason for hiding this comment

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

nitpicking

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in e0a2432

@@ -71,7 +71,7 @@ public class ResourceStepTest {
initFixedContentLibrary();

LibraryConfiguration libraryConfig = new LibraryConfiguration("stuff", new SCMSourceRetriever(new GitSCMSource(null, sampleRepo.toString(), "", "*", "", true)));
libraryConfig.setCachingConfiguration(new LibraryCachingConfiguration(0, ""));
libraryConfig.setCachingConfiguration(new LibraryCachingConfiguration(0, "",""));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
libraryConfig.setCachingConfiguration(new LibraryCachingConfiguration(0, "",""));
libraryConfig.setCachingConfiguration(new LibraryCachingConfiguration(0, "", ""));

Copy link
Contributor

Choose a reason for hiding this comment

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

nitpicking

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in e0a2432

@sboardwell
Copy link
Contributor

Looks good. Hoping someone can find time to review this.

@AnishaGharat
Copy link
Contributor Author

Looks good. Hoping someone can find time to review this.

Thank you so much @sboardwell for the review.
Please do let us know if anything more has to be done from our end.

@car-roll car-roll requested a review from a team December 12, 2022 16:42
@car-roll car-roll added the enhancement New feature or request label Dec 12, 2022

private static final String VERSIONS_SEPARATOR = " ";
public static final String GLOBAL_LIBRARIES_DIR = "global-libraries-cache";
public static final String LAST_READ_FILE = "last_read";

@DataBoundConstructor public LibraryCachingConfiguration(int refreshTimeMinutes, String excludedVersionsStr) {
@DataBoundConstructor public LibraryCachingConfiguration(int refreshTimeMinutes, String excludedVersionsStr, String includedVersionsStr) {
Copy link
Member

Choose a reason for hiding this comment

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

To keep backwards binary compatibility the old constructor signature should be kept and @Deprecated and call the new constructor with default values.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe even a better approach would be to instead just add a @DataboundSetter for the new field, so that it signals that it is optional and doesn't need to be provided unless needed.

@rsandell
Copy link
Member

This change has been here for a year, so perhaps there is no one able to pick this back up, if that is the case I could try to get it over the finish line, unless anybody objects?

@@ -54,6 +70,17 @@ public Boolean isRefreshEnabled() {
public String getExcludedVersionsStr() {
return excludedVersionsStr;
}
public String getIncludedVersionsStr() {
Copy link
Contributor

Choose a reason for hiding this comment

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

do we want a @CheckForNull here?

Copy link
Member

Choose a reason for hiding this comment

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

There isn't any such annotations on the other one?
It I add one here I feel like I would need to add it everywhere :/

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair point, you can disregard my comment :)

Copy link
Contributor

@sboardwell sboardwell left a comment

Choose a reason for hiding this comment

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

lgtm

@rsandell rsandell merged commit 8ee9ed9 into jenkinsci:master Aug 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants