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: improve OLM support #851

Merged
merged 8 commits into from
Mar 19, 2024
Merged

feat: improve OLM support #851

merged 8 commits into from
Mar 19, 2024

Conversation

metacosm
Copy link
Member

  • feat: automatically add provider name and URL based on user and git repo
  • feat(ci): validate all bundles except Joke that OLM v1 doesn't like
  • feat: split name into bundle and csv names to avoid confusion
  • fix: sanitize version

Signed-off-by: Chris Laprun <claprun@redhat.com>
Signed-off-by: Chris Laprun <claprun@redhat.com>
Fixes #738

Signed-off-by: Chris Laprun <claprun@redhat.com>
Signed-off-by: Chris Laprun <claprun@redhat.com>
@metacosm metacosm self-assigned this Mar 15, 2024
@metacosm metacosm requested a review from csviri March 15, 2024 15:23
Signed-off-by: Chris Laprun <claprun@redhat.com>
Signed-off-by: Chris Laprun <claprun@redhat.com>
Signed-off-by: Chris Laprun <claprun@redhat.com>
@metacosm
Copy link
Member Author

Fixes #738

@@ -127,6 +139,37 @@ CSVMetadataBuildItem gatherCSVMetadata(KubernetesConfig kubernetesConfig,
return new CSVMetadataBuildItem(csvGroups);
}

private static String getDefaultProviderURLFromSCMInfo(ApplicationInfoBuildItem appConfiguration,
JarBuildItem jarBuildItem) {
final var maybeProject = KubernetesCommonHelper.createProject(appConfiguration, Optional.empty(),
Copy link
Contributor

Choose a reason for hiding this comment

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

@metacosm need to be extremely careful here. This class uses Dekorate to get the URL info directly from local git config. Personally I consider this unexpected (compared to using VCS info from pom configuration) and it was also the source of the recent Quarkus CVE .

return maybeProject.map(project -> {
final var scmInfo = project.getScmInfo();
if (scmInfo != null) {
var origin = scmInfo.getRemote().get("origin");
Copy link
Contributor

Choose a reason for hiding this comment

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

Again.. same as with that CVE. We should make sure that this can be explicitly overridable -- there are cases when origin might point actually be a fork which one doesn't want to publicly advertise (e.g. when builds are for whatever reason using a mirror URL or when you simply have manual steps in your workflow)

Copy link

Choose a reason for hiding this comment

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

I absolutely must echo @jcechace concern here. In almost all use cases, origin for me is my fork, and upstream refers to the public repository.

Copy link
Member Author

@metacosm metacosm Mar 19, 2024

Choose a reason for hiding this comment

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

Yes, it's very tricky so maybe I'll just remove this and let people deal with it using @CSVMetadata.Provider instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

Again.. same as with that CVE. We should make sure that this can be explicitly overridable -- there are cases when origin might point actually be a fork which one doesn't want to publicly advertise (e.g. when builds are for whatever reason using a mirror URL or when you simply have manual steps in your workflow)

Note that this is for the default value and is overridable using @CSVMetadata and its provider field. That said, this computed value might be faulty in many situations so it's best to remove it.

Signed-off-by: Chris Laprun <claprun@redhat.com>
@metacosm metacosm merged commit 52b9884 into main Mar 19, 2024
6 checks passed
@metacosm metacosm deleted the add-provider-name branch March 19, 2024 20:17
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