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

Enable CD on plugin-compat-tester & clean up obsolete modules #2103

Merged
merged 1 commit into from Oct 17, 2021

Conversation

jglick
Copy link
Contributor

@jglick jglick commented Sep 28, 2021

Description

While working on jenkinsci/bom#659 I realized we are still waiting for some person to mvn -f plugin-compat-tester release:{prepare,perform} on occasion, which makes little sense for a developer tool—we should just be able to use whatever is in master.

Also I am removing modules unused as of jenkinsci/plugin-compat-tester#193.

CC @imonteroperez as apparent primary maintainer, though only @raul-arabaolaza is listed in RPU for now.

Reviewer checklist (not for requesters!)

  • Check this if newly added person also needs to be given merge permission to the GitHub repo (please @ the people/person with their GitHub username in this issue as well). If needed, it can be done using an IRC Bot command
  • Check that the $pluginId Developers team has Admin permissions while granting the access.
  • In the case of plugin adoption, ensure that the Jenkins Jira default assignee is either removed or changed to the new maintainer.
  • If security contacts are changed (this includes add/remove), ping the security officer (currently @daniel-beck) in this pull request. If an email contact is changed, wait for approval from the security officer.

There are IRC Bot commands for it

@jglick jglick requested a review from a team as a code owner September 28, 2021 17:59
@jglick
Copy link
Contributor Author

jglick commented Sep 28, 2021

Also CC @bmunozm, active in last year (but again not currently authorized in RPU).

@@ -3,6 +3,8 @@ name: "plugins-compat-tester-aggregator"
github: "jenkinsci/plugin-compat-tester"
paths:
- "org/jenkins-ci/tests/plugins-compat-tester-aggregator"
cd:
Copy link
Member

Choose a reason for hiding this comment

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

needs to be combined in one file for multi module to work iirc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mm, maybe. #1747. @daniel-beck any idea? Would it suffice as a workaround to just add a cd section to one component file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While

List<Definition> definitions = cdEnabledComponentsByGitHub[definition.github]
if (!definitions) {
definitions = new ArrayList<>()
cdEnabledComponentsByGitHub[definition.github] = definitions
}
LOGGER.log(Level.INFO, "CD-enabled component '${definition.name}' in repository '${definition.github}'")
definitions.add(definition)
captures each YAML file associated with a given repo and cd.enabled, it seems from
cdEnabledComponentsByGitHub.each { githubRepo, components ->
def groupName = ArtifactoryAPI.getInstance().toGeneratedGroupName(githubRepo)
File outputFile = new File(new File(apiOutputDir, 'groups'), groupName + '.json')
JsonBuilder json = new JsonBuilder()
json {
name groupName
description "CD group with permissions to deploy from ${githubRepo}"
}
String pretty = json.toPrettyString()
outputFile.parentFile.mkdirs()
outputFile.text = pretty
}
and that the values are actually unused. IOW it should not even matter—so long as at least one file related to this repo enables CD, we should get the right user and secrets created—though it would have been clearer for to be a TreeSet<String>.

@timja
Copy link
Member

timja commented Sep 28, 2021

Looking at the API:
https://www.jfrog.com/confluence/display/JFROG/Security+Configuration+JSON#SecurityConfigurationJSON-application/vnd.org.jfrog.artifactory.security.PermissionTarget+json

We're assigning the group to the special user that gets created which has an includes pattern.

As far as I can tell that include pattern will not be applied to the right paths unless I'm understanding wrong

@timja
Copy link
Member

timja commented Sep 28, 2021

I think it's a last one wins sort of thing from reading it, the group name for each will be normalised to the same group as it's keyed off the GitHub repo, but the paths that are used will likely be the last one

@jglick
Copy link
Contributor Author

jglick commented Sep 28, 2021

My reading of

if (definition.cd?.enabled) {
groups([(ArtifactoryAPI.getInstance().toGeneratedGroupName(definition.github)): ["w", "n"]])
is that each repo will get one generated group name, but this group might be added as a principal to multiple paths, one per source file and thus permissions/*.json output.

@timja
Copy link
Member

timja commented Sep 28, 2021

ah possibly, we can try it anyway.

@jglick
Copy link
Contributor Author

jglick commented Sep 28, 2021

$ (mvn -DskipTests package && java -DdryRun=true -DdefinitionsDir=$PWD/permissions -DartifactoryApiTempDir=$PWD/json -DartifactoryUserNamesJsonListUrl=https://reports.jenkins.io/artifactory-ldap-users-report.json -Djava.util.logging.SimpleFormatter.format="%1$tY-%1$tm-%1$td %1$tH:%1$tM:%1$tS %4$s: %5$s%6$s%n" -jar target/repository-permissions-updater-*-bin/repository-permissions-updater-*.jar) 2>&1 | fgrep plugin-compat-tester
2021-09-28 19:58:04.293+0000 [id=1]	INFO	o.c.g.r.c.PojoMetaMethodSite$PojoCachedMethodSiteNoUnwrap#invoke: CD-enabled component 'plugins-compat-tester-cli' in repository 'jenkinsci/plugin-compat-tester'
2021-09-28 19:58:04.435+0000 [id=1]	INFO	o.c.g.r.c.PojoMetaMethodSite$PojoCachedMethodSiteNoUnwrap#invoke: CD-enabled component 'plugins-compat-tester' in repository 'jenkinsci/plugin-compat-tester'
2021-09-28 19:58:04.658+0000 [id=1]	INFO	o.c.g.r.c.PojoMetaMethodSite$PojoCachedMethodSiteNoUnwrap#invoke: CD-enabled component 'plugins-compat-tester-aggregator' in repository 'jenkinsci/plugin-compat-tester'
2021-09-28 19:58:04.826+0000 [id=1]	INFO	o.c.g.r.c.PojoMetaMethodSite$PojoCachedMethodSiteNoUnwrap#invoke: CD-enabled component 'plugins-compat-tester-model' in repository 'jenkinsci/plugin-compat-tester'
2021-09-28 19:58:05.155+0000 [id=1]	INFO	o.c.g.r.c.PojoMetaMethodSite$PojoCachedMethodSiteNoUnwrap#invoke: Dry-run mode: Skipping PUT call to https://repo.jenkins-ci.org/api/security/groups/generatedv2-cd-jenkinsci_plugin-compat-tester
2021-09-28 19:58:05.288+0000 [id=1]	INFO	o.c.g.r.c.PojoMetaMethodSite$PojoCachedMethodSiteNoUnwrap#invoke: Processing repository jenkinsci/plugin-compat-tester for CD
2021-09-28 19:58:05.288+0000 [id=1]	INFO	o.c.g.r.c.PojoMetaMethodSite$PojoCachedMethodSiteNoUnwrap#invoke: Skipped creation of token for GitHub repo: 'jenkinsci/plugin-compat-tester', Artifactory user: 'CD-for-jenkinsci__plugin-compat-tester', group name: 'generatedv2-cd-jenkinsci_plugin-compat-tester', valid for 14400 seconds

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.

Want this merged since you’re listed here or wait for someone else?

@jglick
Copy link
Contributor Author

jglick commented Sep 28, 2021

In json/, we have github.index.json with

    "jenkinsci/plugin-compat-tester": [
        "org/jenkins-ci/tests/plugins-compat-tester",
        "org/jenkins-ci/tests/plugins-compat-tester-aggregator",
        "org/jenkins-ci/tests/plugins-compat-tester-cli",
        "org/jenkins-ci/tests/plugins-compat-tester-model"
    ],

and cd.index.json with

    "jenkinsci/plugin-compat-tester",

and groups/generatedv2-cd-jenkinsci_plugin-compat-tester.json with

{
    "name": "generatedv2-cd-jenkinsci_plugin-compat-tester",
    "description": "CD group with permissions to deploy from jenkinsci/plugin-compat-tester"
}

and then permissions/generatedv2-component-plugins-compat-tester{,-aggregator,-cli,-model}.json with e.g.

{
    "name": "generatedv2-component-plugins-compat-tester-cli",
    "includesPattern": "org/jenkins-ci/tests/plugins-compat-tester-cli/*/plugins-compat-tester-cli-*,org/jenkins-ci/tests/plugins-compat-tester-cli/*/maven-metadata.xml,org/jenkins-ci/tests/plugins-compat-tester-cli/*/maven-metadata.xml.*,org/jenkins-ci/tests/plugins-compat-tester-cli/maven-metadata.xml,org/jenkins-ci/tests/plugins-compat-tester-cli/maven-metadata.xml.*",
    "excludesPattern": "",
    "repositories": [
        "snapshots",
        "releases"
    ],
    "principals": {
        "users": {
            "kwhetstone": [
                "w",
                "n"
            ],
            "jglick": [
                "w",
                "n"
            ],
            "andresrc": [
                "w",
                "n"
            ],
            "rarabaolaza": [
                "w",
                "n"
            ],
            "oleg_nenashev": [
                "w",
                "n"
            ]
        },
        "groups": {
            "generatedv2-cd-jenkinsci_plugin-compat-tester": [
                "w",
                "n"
            ]
        }
    }
}

which seems like it should work.

I am also tempted to just deploy only the plugins-compat-tester-cli, which is the thing you actually need. The other things are just intermediate build products.

@jglick
Copy link
Contributor Author

jglick commented Sep 28, 2021

Want this merged since you’re listed here or wait for someone else?

I guess it can be merged, provided that you are able to watch the deployment on trusted CI in case something blows up.

Copy link
Contributor

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

+1 from me as a maintainer. CD is helpful there

@oleg-nenashev oleg-nenashev merged commit 9501ef6 into jenkins-infra:master Oct 17, 2021
@jglick jglick deleted the plugin-compat-tester branch October 18, 2021 12:55
@jglick
Copy link
Contributor Author

jglick commented Oct 18, 2021

I will try to prep this as soon as I get a moment.

@@ -3,6 +3,8 @@ name: "plugins-compat-tester-cli"
github: "jenkinsci/plugin-compat-tester"
paths:
- "org/jenkins-ci/tests/plugins-compat-tester-cli"
cd:
enabled: true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note jenkinsci/plugin-compat-tester#320 (comment): could perhaps omit CD on the other modules, or if jenkinsci/plugin-compat-tester#320 is merged, simply remove the other modules from RPU altogether (would only ever be installed locally).

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

3 participants