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-56805] Support Configuration as Code #48

Merged
merged 39 commits into from
Apr 24, 2019
Merged

[JENKINS-56805] Support Configuration as Code #48

merged 39 commits into from
Apr 24, 2019

Conversation

ndeloof
Copy link
Contributor

@ndeloof ndeloof commented May 16, 2018

Move configuration-as-code support in matrix-auth plugin

Configuration-as-Code JEP-201 allow to configure a full jenkins master from a plain text definition. We'd like this feature hosted by the implementor.

see jenkinsci/configuration-as-code-plugin#197

@daniel-beck
Copy link
Member

daniel-beck commented Jul 10, 2018

@ndeloof Sorry for the late response. This PR doesn't build (anymore), could you please look into that?

The test and sample config look reasonable, but please fix up license headers & Javadoc.

There's also #46 and I need your and @MadsNielsen's help to figure out which is the better approach, as I'm not sure what's going on exactly -- are they alternatives, if so, what are the pros/cons? Or do they complement each other?

Is there some synergy for CasC and Job DSL (#45) to be had, or are they too dissimilar for that? CC @daspilker

@ndeloof
Copy link
Contributor Author

ndeloof commented Jul 10, 2018

I'm not sure about #46 intent, I haven't seen the "JCasC side" of this PR. But if we get Configurator hosted in plugin this probably would make more sense.

About #45 I don't know Job DSL enough to tell you the exact impact but I guess it relies on comparable mechanisms, just focussing on job configuration. We both need the same level of "cleanup" in jenkins plugins :)

Will update this PR once 0.10-alpha has been released.

@daniel-beck
Copy link
Member

@ndeloof Thanks for the explanation. Please ping me out of band if I miss the PR update.

@ndeloof
Copy link
Contributor Author

ndeloof commented Jul 11, 2018

updated with some changes :
custom converter now uses JCasC API (vs fully reimplementing configure) and uses a flatten permission list model :

    projectMatrix:
      grantedPermissions:
        - "Overall/Read:anonymous"
        - "Overall/Administer:authenticated"

waiting for 0.10-alpha release

@daniel-beck
Copy link
Member

Still failing. Please specify the snapshot dependency using a timestamp for the artifact you uploaded.

@ndeloof
Copy link
Contributor Author

ndeloof commented Jul 12, 2018

I don't understand this access modifier failure :-\

@daniel-beck
Copy link
Member

@ndeloof Most were due to outdated parent POM.

@daniel-beck
Copy link
Member

Test failures since the configuration-as-code snapshot still includes the classes now moved here?

@ndeloof
Copy link
Contributor Author

ndeloof commented Jul 13, 2018

cyclic dependency, as I should have anticipated. Need to remove dependency from JCasC

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.

Looks mighty fine 👍

@daniel-beck
Copy link
Member

What's the plan to release this + that? Ideally I'd like to give this a spin without time pressure while a release of CasC is out that doesn't support matrix-auth anymore, or are your early adopters unusually accepting of breakage?

@jetersen
Copy link
Member

jetersen commented Jul 13, 2018

As an early adopter I say breakage is expected 👍 It is still alpha for that reason

@ndeloof
Copy link
Contributor Author

ndeloof commented Jul 13, 2018

@daniel-beck I'd expect custom configurator removed for JCasC in next alpha release (some weeks at most).
We broke things few times already, so no pressure :)

@jetersen
Copy link
Member

I moved the current CI issue into #51

@daniel-beck daniel-beck dismissed their stale review April 22, 2019 00:42

obsolete

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.

Tested this out on a clone of my production setup,
Existing vars worked, and logged the deprecation warning:

 WARNING: Loading deprecated attribute 'grantedPermissions' for instance of 'hudson.security.GlobalMatrixAuthorizationStrategy'. Use 'permissions' instead.

and if I update it I no longer get the warning
Also tested modifying the config and viewing the export at:
http://localhost:8088/configuration-as-code/viewExport

looks good

great work @daniel-beck

@timja
Copy link
Member

timja commented Apr 22, 2019

No incrementals due to:

Skipping deployment as no artifacts were found with the expected path, typically due to a PR merge build not up to date with its base branch

I assume master needs merging in for an incrementals build?

@daniel-beck
Copy link
Member

@timja I did that, deployment is https://repo.jenkins-ci.org/incrementals/org/jenkins-ci/plugins/matrix-auth/2.4-rc247.5d90c4bd1f21/

@timja
Copy link
Member

timja commented Apr 22, 2019

Nice, anything left to do on this?

@daniel-beck
Copy link
Member

Ideally more reviews confirming I didn't take a wrong turn somewhere, since I'm not a user of any of this myself.

@daniel-beck daniel-beck requested a review from jetersen April 23, 2019 00:01
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 one oddity but it is super minor.

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.

9 participants