Skip to content

Conversation

@rnc
Copy link
Collaborator

@rnc rnc commented Jul 23, 2024

No description provided.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 23, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@rnc rnc requested a review from vibe13 July 23, 2024 13:48
}

// TODO: ### For container-builds, should sbom generation be delegated to the task within that? If it supports it?
private void generateBuildSbom() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@vibe13 To discuss ; Previously JBS handled SBOM after the build - I suspect now Konflux should handle that?

Copy link
Member

Choose a reason for hiding this comment

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

Mmmm no sure about Konflux handling java SBOMs. Rather, SBOMer could be used because it can generate good quality manifests. But, JBS has the capability to manifest the contaminated GAVs and contaminants, so we can think of JBS creating a skeleton SBOM to be later enriched by SBOMer. I would keep it in for now, then we will discuss with Marek and see how to design this. Good point!

Copy link
Member

Choose a reason for hiding this comment

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

But, I would like to see the content of SBOMs generated by Konflux regardless. There were discussions about merging our generated SBOMs with Konflux generated ones.

Copy link
Collaborator Author

@rnc rnc Jul 25, 2024

Choose a reason for hiding this comment

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

At the moment we don't have any way of handling / storing the sbom - this was handled by BuildVerifyCommand but as we don't have a way as far as I know in the buildah based task to access the .m2/gradle repositories (as we don't have a shared workspace) I think this now needs to be handled specifically by Konflux.

// //{Name: WorkspaceBuildSettings, Workspace: WorkspaceBuildSettings},
// {Name: WorkspaceSource, Workspace: WorkspaceSource},
// //{Name: WorkspaceTls, Workspace: WorkspaceTls},
//},
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Currently I haven't figured out how (if at all) we can pass the setting.xml/tls shared workspaces - I suspect we'll need a different approach.

Copy link
Member

Choose a reason for hiding this comment

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

ok

@codecov
Copy link

codecov bot commented Jul 23, 2024

Codecov Report

Attention: Patch coverage is 75.00000% with 66 lines in your changes missing coverage. Please review.

Project coverage is 39.51%. Comparing base (18b09dd) to head (739d5ee).
Report is 20 commits behind head on main.

Files with missing lines Patch % Lines
pkg/reconciler/dependencybuild/buildrecipeyaml.go 75.59% 58 Missing and 4 partials ⚠️
...edhat/hacbs/container/deploy/TagDeployCommand.java 0.00% 2 Missing ⚠️
...hat/hacbs/container/deploy/BuildVerifyCommand.java 0.00% 1 Missing ⚠️
pkg/reconciler/dependencybuild/dependencybuild.go 80.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #1977      +/-   ##
============================================
+ Coverage     39.38%   39.51%   +0.13%     
+ Complexity      812      811       -1     
============================================
  Files           301      301              
  Lines         14040    14145     +105     
  Branches       1469     1467       -2     
============================================
+ Hits           5529     5589      +60     
- Misses         7861     7907      +46     
+ Partials        650      649       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@vibe13 vibe13 left a comment

Choose a reason for hiding this comment

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

I have put some more questions for the sake of my understanding but overall it makes sense to me, thanks Nick!

}

// TODO: ### For container-builds, should sbom generation be delegated to the task within that? If it supports it?
private void generateBuildSbom() {
Copy link
Member

Choose a reason for hiding this comment

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

Mmmm no sure about Konflux handling java SBOMs. Rather, SBOMer could be used because it can generate good quality manifests. But, JBS has the capability to manifest the contaminated GAVs and contaminants, so we can think of JBS creating a skeleton SBOM to be later enriched by SBOMer. I would keep it in for now, then we will discuss with Marek and see how to design this. Good point!

}

// TODO: ### For container-builds, should sbom generation be delegated to the task within that? If it supports it?
private void generateBuildSbom() {
Copy link
Member

Choose a reason for hiding this comment

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

But, I would like to see the content of SBOMs generated by Konflux regardless. There were discussions about merging our generated SBOMs with Konflux generated ones.

// //{Name: WorkspaceBuildSettings, Workspace: WorkspaceBuildSettings},
// {Name: WorkspaceSource, Workspace: WorkspaceSource},
// //{Name: WorkspaceTls, Workspace: WorkspaceTls},
//},
Copy link
Member

Choose a reason for hiding this comment

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

ok

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 25, 2024

New changes are detected. LGTM label has been removed.

@rnc rnc force-pushed the PIPELINE branch 4 times, most recently from 2ff591a to 9d5d9c4 Compare July 26, 2024 12:13
@rnc rnc force-pushed the PIPELINE branch 2 times, most recently from dc28e32 to b94f702 Compare July 31, 2024 08:31
@rnc rnc marked this pull request as ready for review August 1, 2024 08:55
@rnc rnc force-pushed the PIPELINE branch 3 times, most recently from a536235 to b3ccee1 Compare August 1, 2024 09:47
@rnc
Copy link
Collaborator Author

rnc commented Aug 1, 2024

/retest

@rnc rnc force-pushed the PIPELINE branch 2 times, most recently from 1eda89f to 5ac7bec Compare August 16, 2024 11:40
@rnc rnc merged commit 7beffb7 into redhat-appstudio:main Aug 30, 2024
@rnc rnc deleted the PIPELINE branch August 30, 2024 11:35
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