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

[JEP-229] Generate temporary Artifactory tokens and submit them to GitHub repos for CD #1747

Merged
merged 16 commits into from Dec 12, 2020

Conversation

daniel-beck
Copy link
Contributor

@daniel-beck daniel-beck commented Nov 6, 2020

See JEP-229.

This adds a new optional element to YAML files,

cd:
  enabled: true

If set and enabled, 1) a group will be generated in Artifactory, 2) the group is added to the permissions target for the artifact, and 3) a token for the group is generated. This required quite a bit of rework, since so far we managed upload permissions entirely without groups. Now we also need to generate those, because anonymous Artifactory tokens require groups to get permissions. To limit the amount of generated data and make permissions easier to trouble-shoot, the groups created here will always be empty, and only groups for CD-enabled artifacts will be created. Once all of the above is done, the generated token is uploaded to the GitHub repository as a secret.

To not interfere with the previous iteration of the tool, a new prefix generatedv2 is used for Artifactory permission targets and groups. While in development, this also does not assign real release permissions; instead it operates on the snapshots repository.

WIP checklist:

"Maybe" list:

  • Error handling for HTTP requests that actually works
  • There may (finally) be tests, TBD

Stuff that's not being fixed:

  • This doesn't fix the various problems with the current file format, e.g.
    • Multi-module projects are distributed across many files, making permission management error-prone
    • Redundancy in name and paths that leads to errors
  • In fact, the first item above is made worse here, this might create tokens that can only upload a subset of artifacts in a repo.

FYI @jglick

Comment on lines +39 to 40
env.JAVA_HOME = tool 'jdk11'
sh "${mvnHome}/bin/mvn -U clean verify"
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
env.JAVA_HOME = tool 'jdk11'
sh "${mvnHome}/bin/mvn -U clean verify"
withEnv(["JAVA_HOME=${tool 'jdk11'}", "PATH+MVN=$mvnHome"]) {
sh 'mvn -U clean verify'
}

Copy link
Contributor

Choose a reason for hiding this comment

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

see also jenkinsci/workflow-cps-plugin#370 and JENKINS-28718 (automatic in Declarative)


@Override
@NonNull String toGeneratedGroupName(String baseName) {
// Add 'cd' to indicate this group is for CD only
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that the token can be used for non-continuous (on-demand) delivery as well; it is more about automated publishing. Maybe too fine a distinction.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, but the idea here is to help people in a few years better understand what these groups are for through a name that hints at the use. That the "CD" infra can be used to deploy in non-CD ways, granted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Also cd is the name of the YAML element that enables the group creation. If you have better suggestions, please tell me.)

@jglick
Copy link
Contributor

jglick commented Nov 30, 2020

For my reference, the GitHub secret names changed as of f7899b8 to not mention Artifactory specifically.

@jglick
Copy link
Contributor

jglick commented Nov 30, 2020

Is there a plan to make this live?

@daniel-beck
Copy link
Contributor Author

IMO we can squash-merge this at any time :) It's backward compatible with existing stuff and we can carefully onboard new repos.

@daniel-beck daniel-beck merged commit e449810 into jenkins-infra:master Dec 12, 2020
@daniel-beck
Copy link
Contributor Author

daniel-beck commented Dec 12, 2020

Looks like nobody else cared, so I merged this.

Slight trouble on trusted.ci as there was no tool jdk11 😞 The complete lack of error messages by the tool step didn't help either.

I checked the results and they look good, so I will proceed with removing the now obsolete generated- permission targets in the next few days. Given the size of this change, some people may be losing upload permissions. Please inform me about permission problems.

@jglick
Copy link
Contributor

jglick commented Dec 14, 2020

So, this is live and ready to use now, and I can proceed with JEP-229 testing?

the GitHub secret names changed

https://github.com/jglick/jenkins-maven-cd-action/issues/2

@timja
Copy link
Member

timja commented Dec 14, 2020

Yes

@daniel-beck
Copy link
Contributor Author

So, this is live and ready to use now, and I can proceed with JEP-229 testing?

Sort of, the pipeline is broken, it does not grant access to a GH token. I asked @olblak on IRC what the scopes of credentials in trusted.ci are, to amend the pipeline, but no answer so far.

Once we fix that, we can add cd: true to your plugin and it should work.

@jglick
Copy link
Contributor

jglick commented Dec 14, 2020

OK, just let me know when you believe it is actually working.

@timja
Copy link
Member

timja commented Dec 14, 2020

jenkins-infra-bot-github-token has been added to trusted CI. Can you give that a try?

@timja
Copy link
Member

timja commented Mar 23, 2021

In fact, the first item above is made worse here, this might create tokens that can only upload a subset of artifacts in a repo.

does this handle multi-module repoes?

@daniel-beck
Copy link
Contributor Author

daniel-beck commented Mar 23, 2021

does this handle multi-module repoes?

I doubt it. Something for a future enhancement I think.

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.

None yet

4 participants