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

feat(build): add buildSettingsInfo for gradleExecuteBuild #5043

Merged
merged 12 commits into from
Oct 15, 2024

Conversation

anilkeshav27
Copy link
Member

Changes

build settings metadata extension for the gradle build step

  • Tests
  • Documentation

@anilkeshav27
Copy link
Member Author

/it-go

@anilkeshav27 anilkeshav27 marked this pull request as ready for review September 12, 2024 11:58
@anilkeshav27 anilkeshav27 requested a review from a team as a code owner September 12, 2024 11:58
@vstarostin
Copy link
Member

this needs to be changed, should be gradleExecuteBuild

@vstarostin
Copy link
Member

and just a comment, up to you if you want to change It as well: buildTool is a little strange name here, because it is actually step name. We can rename to stepName, buildStep ...

@CCFenner CCFenner changed the title feat (build settings) for gradleExecuteBuild feat(build): add buildSettingsInfo for gradleExecuteBuild Sep 13, 2024
cmd/gradleExecuteBuild.go Outdated Show resolved Hide resolved
}
buildSettingsInfo, err := buildsettings.CreateBuildSettingsInfo(&gradleConfig, stepNameForBuildSettings)
if err != nil {
log.Entry().Warnf("failed to create build settings info: %v", err)
Copy link
Member

Choose a reason for hiding this comment

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

Why do we fail above, but not here?

Copy link
Member Author

Choose a reason for hiding this comment

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

the buildSettings info gathering can fail, but that must fail the step, its a compliance information fetch fail, but i am not sure if the entire step should fail for that reason ?

Copy link
Member

Choose a reason for hiding this comment

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

It works the same way across all build steps.
If we have a warning instead of err, in case the method above fails, dockerImage in build-settings will be an empty string, but in fact gradle image is used.
It is compliance information, not sure if having wrong docker image info is ok.

@vstarostin
Copy link
Member

/it-go

1 similar comment
@vstarostin
Copy link
Member

/it-go

@vstarostin vstarostin force-pushed the anil/gradleBuildSettings branch from 6941063 to 63df010 Compare October 15, 2024 06:53
Copy link

@vstarostin
Copy link
Member

/it-go

@vstarostin vstarostin merged commit bc8225c into master Oct 15, 2024
20 checks passed
@vstarostin vstarostin deleted the anil/gradleBuildSettings branch October 15, 2024 07:14
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.

4 participants