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

Add possibility to reload secrets from properties file with JCasC reload #1556

Merged
merged 4 commits into from Jan 20, 2021

Conversation

tempora-mutantur
Copy link
Contributor

Your checklist for this pull request

🚨 Please review the guidelines for contributing to this repository.

  • Make sure you are requesting to pull a topic/feature/bugfix branch (right side) and not your master branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or in Jenkins JIRA - Add possibility to reload secrets from properties file with JCasC reload #1555
  • Did you provide a test-case? That demonstrates feature works or fixes the issue. - checked by changing the properties file

@tempora-mutantur tempora-mutantur requested a review from a team as a code owner January 20, 2021 00:00
@codecov
Copy link

codecov bot commented Jan 20, 2021

Codecov Report

Merging #1556 (1edab60) into master (9a40a90) will decrease coverage by 0.00%.
The diff coverage is 75.00%.

@@             Coverage Diff              @@
##             master    #1556      +/-   ##
============================================
- Coverage     81.02%   81.01%   -0.01%     
+ Complexity      811      810       -1     
============================================
  Files            66       66              
  Lines          2371     2370       -1     
  Branches        329      328       -1     
============================================
- Hits           1921     1920       -1     
  Misses          349      349              
  Partials        101      101              
Impacted Files Coverage Δ Complexity Δ
...gins/casc/impl/secrets/PropertiesSecretSource.java 80.00% <75.00%> (-1.25%) 6.00 <4.00> (-1.00)

@jetersen jetersen self-requested a review January 20, 2021 04:06
Copy link
Member

@jetersen jetersen left a comment

Choose a reason for hiding this comment

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

LGTM

@timja
Copy link
Member

timja commented Jan 20, 2021

@tempora-mutantur any chance of a test that validates that secrets are reloaded when the properties change? codecov is complaining of a lack of test coverage here

@timja timja added the feature A PR that adds a feature - used by Release Drafter label Jan 20, 2021
@jetersen
Copy link
Member

jetersen commented Jan 20, 2021

@timja The lack of test coverage is actually due to file exist if condition and catching exception which is kinda 🤷‍♂️

See codecoverage diff

@timja
Copy link
Member

timja commented Jan 20, 2021

@timja The lack of test coverage is actually due to file exist if condition and catching exception which is kinda 🤷‍♂️

See codecoverage diff

well I assume we aren't testing that you can change the file and reload and it works since that didn't work before.

having the secret source not fail when the file is missing having a test wouldn't be a bad thing, not that I'm convinced it's a good feature, personally I would rather it just fails but maybe people rely on that now.

not a blocker

@jetersen
Copy link
Member

jetersen commented Jan 20, 2021

I agree with you on the need for a properties reload test would be great.

Just correcting as to why codecov was complaining since codecov has changed the detail link to point to their homepage and not the pull request link.

@timja timja merged commit 161c501 into jenkinsci:master Jan 20, 2021
@timja
Copy link
Member

timja commented Jan 20, 2021

Thanks, codecov is still complaining about missing coverage on no properties file but meh!

@tempora-mutantur tempora-mutantur deleted the issue-1555 branch January 21, 2021 11:10
@martinda
Copy link
Contributor

T@tempora-mutantur Is there a documentation update please? I'd like to understand how to use this feature. Thanks.

@jetersen
Copy link
Member

jetersen commented Feb 15, 2021

This is not something that is user facing @martinda next time the configure is called JCASC will read the properties again using the init method.
It should be as simply as updating the file and hit configure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature A PR that adds a feature - used by Release Drafter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants