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.

Sign up for GitHub

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

Jump to bottom

sort Attributes and RootElementConfigurator according to Extension ordinal #1394

Merged
merged 6 commits into from
May 19, 2020
Merged

sort Attributes and RootElementConfigurator according to Extension ordinal #1394

merged 6 commits into from
May 19, 2020

Conversation

jetersen
Copy link
Member

@jetersen jetersen commented May 19, 2020 •
edited
Loading

This ensures for instance Credentials are loaded first.
Jenkins Configurator is loaded second.
Seed Job is loaded last.

Testing this is pretty hard due to how Jenkins Test harness fixes the Jenkins location url 😓
But as shown below the sorting works and should allow some sense of order when configuring location and resourceRoot.

Before:
image

After:
image

Here you have RootElementConfigurator after, just imagine the the before as an totally unsorted list 🤣
image

fixes #1383
fixes #619
fixes #876

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
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Did you provide a test-case? That demonstrates feature works or fixes the issue.

@jetersen jetersen requested a review from a team as a code owner May 19, 2020 07:04
jetersen
jetersen commented May 19, 2020
View reviewed changes
Copy link
Member Author

@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.

One separate improvement I could see is double check that RootElementConfigurator are sorted after ordinal as well, this should solve the many issues we had with job dsl sometimes loading too soon.

plugin/src/main/java/io/jenkins/plugins/casc/BaseConfigurator.java Outdated
@@ -298,7 +301,8 @@ public T check(CNode c, ConfigurationContext context) throws ConfiguratorExcepti
*/
protected void configure(Mapping config, T instance, boolean dryrun, ConfigurationContext context) throws ConfiguratorException {
final Set<Attribute<T,?>> attributes = describe();
for (Attribute<T,?> attribute : attributes) {
List<Attribute<T, ?>> sortedAttributes = sortAttributes(attributes);
Copy link
Member Author

Choose a reason for hiding this comment

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

As this is the protected configure block I feel safe to say that other plugins providing configurators will also be sorted according to the ordinal.

timja
timja approved these changes May 19, 2020
View reviewed changes
Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

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

lgtm

@peachy-devops do you want to test this to see if it fixes your issue?
You'll be able to download a version from there: https://ci.jenkins.io/blue/organizations/jenkins/Plugins%2Fconfiguration-as-code-plugin/detail/PR-1394/1/artifacts once the build finishes

plugin/src/main/java/io/jenkins/plugins/casc/BaseConfigurator.java Outdated Show resolved Hide resolved
@jetersen @timja
pretty backets
2eb82b7 
Co-authored-by: Tim Jacomb <t.jacomb@kainos.com>
@jetersen jetersen mentioned this pull request May 19, 2020
Closed
6 tasks
@codecov
Copy link

codecov bot commented May 19, 2020 •
edited
Loading

Codecov Report

Merging #1394 into master will increase coverage by 0.07%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master    #1394      +/-   ##
============================================
+ Coverage     79.29%   79.37%   +0.07%     
- Complexity      790      796       +6     
============================================
  Files            66       66              
  Lines          2314     2322       +8     
  Branches        324      322       -2     
============================================
+ Hits           1835     1843       +8     
  Misses          378      378              
  Partials        101      101
Impacted Files Coverage Δ Complexity Δ
...jenkins/plugins/casc/core/JenkinsConfigurator.java 95.74% <ø> (ø) 18.00 <0.00> (ø)
...java/io/jenkins/plugins/casc/BaseConfigurator.java 85.35% <100.00%> (-0.08%) 70.00 <0.00> (ø)
...ain/java/io/jenkins/plugins/casc/Configurator.java 79.06% <100.00%> (+5.54%) 23.00 <6.00> (+6.00)
.../jenkins/plugins/casc/RootElementConfigurator.java 86.66% <100.00%> (+0.95%) 4.00 <0.00> (ø)
...ava/io/jenkins/plugins/casc/TokenReloadAction.java 95.00% <0.00%> (-0.24%) 11.00% <0.00%> (ø%)

@peachy-devops
Copy link

@timja I'll try if I can. Can you please provide me the steps on how install it using a dockerfile? Not sure where to insert the hpi file for Jenkins to install it. I tried updating via GUI and updated successfully but I need to have it on a dockerfile.

Here is the dockerfile I sent to @jetersen

FROM jenkins/jenkins:lts
LABEL maintainer="man@praqma.net" 

ARG JAVA_OPTS
ENV JAVA_OPTS "-Djenkins.install.runSetupWizard=false ${JAVA_OPTS:-}"

##### JENKINS SETUP. For alpha releases use the experimental update center
ENV JENKINS_HOME /var/jenkins_home
ENV CASC_PATH="${JENKINS_HOME}/casc_configs"

COPY plugins.txt /usr/share/jenkins/ref/plugins.txt
RUN xargs /usr/local/bin/install-plugins.sh < /usr/share/jenkins/ref/plugins.txt

COPY jenkins-minimal-example.yaml ${CASC_PATH}/

@timja
Copy link
Member

timja commented May 19, 2020

You can either pass a URL to the plugin or use the incrementals repo:

Sample:
https://github.com/hmcts/cnp-jenkins-docker/blob/master/plugins.txt#L115
https://github.com/hmcts/cnp-jenkins-docker/blob/master/plugins.txt#L34

@jetersen jetersen changed the title Sort attributes according to the ordinal sort Attributes and RootElementConfigurator according to Extension ordinal May 19, 2020
This was referenced May 19, 2020
Closed
Closed
Closed
@jetersen
Copy link
Member Author

😨 Jenkins failed on deploy due to Windows agent exploding!

@jetersen jetersen closed this May 19, 2020
@jetersen jetersen reopened this May 19, 2020
@jetersen
Copy link
Member Author

#1383 (comment)

Works out :)

@jetersen jetersen added the bugfix A PR that fixes a bug - used by Release Drafter label May 19, 2020
@jetersen jetersen merged commit 0f26dd8 into jenkinsci:master May 19, 2020
@jetersen jetersen deleted the feat/ordinal branch May 19, 2020 16:46
@peachy-devops
Copy link

You can either pass a URL to the plugin or use the incrementals repo:

Sample:
https://github.com/hmcts/cnp-jenkins-docker/blob/master/plugins.txt#L115
https://github.com/hmcts/cnp-jenkins-docker/blob/master/plugins.txt#L34

@timja your solution worked! 😄
@jetersen I tested the latest artifact and it works! Looking forward on the official release 👍

@jetersen jetersen mentioned this pull request May 24, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timja timja timja approved these changes

Assignees
No one assigned
Labels
bugfix A PR that fixes a bug - used by Release Drafter
Projects
None yet
Milestone
No milestone
3 participants
@jetersen @peachy-devops @timja