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

CURATOR-669. Integrate with ge.apache.org Gradle Enterprise server #459

Merged
merged 3 commits into from
May 6, 2023

Conversation

tisonkun
Copy link
Member

@tisonkun tisonkun commented Apr 12, 2023

Signed-off-by: tison <wander4096@gmail.com>
@tisonkun
Copy link
Member Author

Copy link
Contributor

@clayburn clayburn left a comment

Choose a reason for hiding this comment

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

Hi, I am a Solutions engineer with Gradle assisting with the Gradle Enterprise rollout. It is exciting to see your interest in sending build scans for the Curator build to https://ge.apache.org! I've added some best practice recommendations to the pull request.

If you have any questions regarding Gradle Enterprise, please feel free to reach out and we would be happy to help you. I see your primary use case is the Test analytics, but after this project is connected, we will run some experiment builds to check how well your build utilizes the build cache and submit back any fixes we discover.

.mvn/ge-extensions.xml Outdated Show resolved Hide resolved
.mvn/gradle-enterprise.xml Show resolved Hide resolved

if (System.env.MAVEN_CMD_LINE_ARGS) {
mavenCommand = "mvn ${System.env.MAVEN_CMD_LINE_ARGS}".toString()
buildScan.value('Maven command line', mavenCommand)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would be very careful capturing the entire Maven command line. You could imagine scenarios where a user passes in a secret as a system property, or possibly even typos that include secrets. Gradle Enterprise doesn't capture certain environment details for this reason.

Perhaps it would make more sense to capture specific details you may be interested in? For example, we already add a custom value for the skipTests setting. We also have samples to capture profiles as tags. This may actually make it easier to queries that combine the different values, rather than trying to construct queries that are considering the command line as a whole. It may also be more accurate, as capturing the value of configured properties would not only capture values passed in at the command line.

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'll leave it as is for now. If you have better config suggestions, feel free to patch the script based on this branch or after this patch gets merged.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. I'm trying to redact mvn command.

@tisonkun - should just be a matter of removing this if (System.env.MAVEN_CMD_LINE_ARGS) { ... } block

.mvn/ge-extensions.xml Outdated Show resolved Hide resolved
@clayburn
Copy link
Contributor

clayburn commented Apr 26, 2023

Just wanted to follow up on this. I know you originally looked at the Gradle Enterprise configuration in the Pulsar project. We've filed a PR there with a couple of changes over there to follow some new recommendations that come with Gradle Enterprise Maven Extension 1.17.

@tisonkun
Copy link
Member Author

@clayburn Thanks for your updates. I'll catch up changes in weeks :)

Signed-off-by: tison <wander4096@gmail.com>
@tisonkun
Copy link
Member Author

@clayburn Updated at a298df5. Please give another look :D

Copy link
Contributor

@clayburn clayburn left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@kezhuw kezhuw left a comment

Choose a reason for hiding this comment

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

Generally looks good to me. I think @clayburn raised a good point about the capture of maven command line especially after enabling local scans. We can enable limited maven command line capture for local scans as what @clayburn suggested in following up pr.

@tisonkun
Copy link
Member Author

tisonkun commented May 5, 2023

OK. I'm trying to redact mvn command.

@tisonkun
Copy link
Member Author

tisonkun commented May 5, 2023

After a closer look, I'm going to remove the if block for now.

Signed-off-by: tison <wander4096@gmail.com>
@clayburn
Copy link
Contributor

clayburn commented May 5, 2023

This test is failing on the Java 8 test build: https://ge.apache.org/s/ovviiad2aixry/tests/overview?outcome=failed

I don't know much about your test suite, but I would assume this one is not related to the changes in this PR?

@tisonkun
Copy link
Member Author

tisonkun commented May 6, 2023

@clayburn Yep. I have rerun the tests and such flaky tests are part of issue we want to located and resolve with the help of Gradle Enterprise integration.

@kezhuw
Copy link
Member

kezhuw commented May 6, 2023

Reported to https://issues.apache.org/jira/browse/CURATOR-671. I saw this also in #460(parallel with this pr). cc @clayburn @tisonkun

@tisonkun
Copy link
Member Author

tisonkun commented May 6, 2023

Pending to merge...

@tisonkun tisonkun merged commit 447878f into master May 6, 2023
@tisonkun tisonkun deleted the CURATOR-669 branch May 6, 2023 03:09
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.

3 participants