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 config as code support for credentials configuration #235

Merged
merged 6 commits into from
Oct 22, 2021

Conversation

timja
Copy link
Member

@timja timja commented Oct 1, 2021

some minor jcasc testing cleanup done as well

Fixes jenkinsci/azure-keyvault-plugin#92

pom.xml Outdated Show resolved Hide resolved
.map(type -> type.getClass().getName())
.filter(classNames::contains)
.collect(Collectors.toList());
return new ArrayList<>(classNames);
Copy link
Member Author

Choose a reason for hiding this comment

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

I couldn't find any references to this method anywhere so I don't know how to test if this works or not, all tests in this plugin pass.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've checked and the functionality for includes and excludes still works:

image

Copy link
Contributor

Choose a reason for hiding this comment

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

💡 What you tested in your screenshot is the CredentialsProviderFilter.
For CredentialsTypeFilter it's the Types > Add > Includes / Excludes.
Manually tested, it still works 👍 (at least, not discovered flaw)

(the last one, Restrictions, is covered by CredentialsProviderTypeRestriction)

Copy link
Contributor

@Wadeck Wadeck left a comment

Choose a reason for hiding this comment

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

I confirm all tests are passing except the export.

Waiting on the companion PR to merge, but I will try to merge other PRs and releasing before to not have incrementals in release.

.map(type -> type.getClass().getName())
.filter(classNames::contains)
.collect(Collectors.toList());
return new ArrayList<>(classNames);
Copy link
Contributor

Choose a reason for hiding this comment

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

👍
With the assertion in mind that the classNames is coming from the UI ("positive" story), that list is populated from getProviderDescriptors which corresponds. No negative story found as the filter method only receive a CredentialsProvider, so no cheat possible.

Only "cons" is if some providers are removed over time, the existing configuration will still have a Set with them inside, no impact.

.map(type -> type.getClass().getName())
.filter(classNames::contains)
.collect(Collectors.toList());
return new ArrayList<>(classNames);
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 What you tested in your screenshot is the CredentialsProviderFilter.
For CredentialsTypeFilter it's the Types > Add > Includes / Excludes.
Manually tested, it still works 👍 (at least, not discovered flaw)

(the last one, Restrictions, is covered by CredentialsProviderTypeRestriction)

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.

Overall looks good to me :)

@timja
Copy link
Member Author

timja commented Oct 1, 2021

Waiting on the companion PR to merge, but I will try to merge other PRs and releasing before to not have incrementals in release.

Propogating through atm, will merge and release bom and update bom, if you're in no rush I'd hold off

pom.xml Outdated Show resolved Hide resolved
@Wadeck
Copy link
Contributor

Wadeck commented Oct 1, 2021

if you're in no rush I'd hold off

Doing some reviews/merge today, if not done today, will come back on it soon-ish after ;)

@timja timja requested review from jglick and Wadeck October 3, 2021 12:31
@timja
Copy link
Member Author

timja commented Oct 11, 2021

@Wadeck reminder

@timja
Copy link
Member Author

timja commented Oct 22, 2021

@jvz / @Wadeck any chance you can take a look

@jglick jglick merged commit e55d6bc into jenkinsci:master Oct 22, 2021
@timja timja deleted the fix-casc-for-cred-provider-filter branch October 22, 2021 15:20
@jglick
Copy link
Member

jglick commented Dec 9, 2021

Possibly caused: https://issues.jenkins.io/browse/JENKINS-67170

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants