Skip to content

Conversation

somtochiama
Copy link
Member

Closes: #flux2/2857

Construct full URL for HelmRepository of OCI type properly when there's a trailing slash in spec.url of the helm repository.

Signed-off-by: Somtochi Onyekwere [email protected]

@somtochiama somtochiama changed the title Remove trailing slash in spec.url of helm re Remove trailing slash in spec.url when getting tags for oci repository Jun 23, 2022
@somtochiama somtochiama requested a review from makkes June 23, 2022 09:25
@darkowlzz darkowlzz added area/helm Helm related issues and pull requests area/oci OCI related issues and pull requests labels Jun 23, 2022
@darkowlzz darkowlzz mentioned this pull request Jun 23, 2022
@darkowlzz
Copy link
Contributor

Let's make this testable in TestOCIChartRepoisitory_Get, also let's fix the typo in the test name:

- TestOCIChartRepoisitory_Get
+ TestOCIChartRepository_Get

The URL in

url := "oci://localhost:5000/my_repo"
can be made an input of the test cases and we can have a case with trailing slash in the URL.

@somtochiama somtochiama requested a review from darkowlzz June 23, 2022 16:00
@darkowlzz darkowlzz requested a review from souleb June 23, 2022 19:28
@somtochiama somtochiama requested a review from souleb June 27, 2022 09:41
Copy link
Member

@souleb souleb left a comment

Choose a reason for hiding this comment

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

LGTM! thanks @somtochiama

Copy link
Contributor

@darkowlzz darkowlzz left a comment

Choose a reason for hiding this comment

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

Let's remove all the changes from controllers/helmchart_controller.go as they have been add in #770 and keep this PR focused on oci_chart_repository.go and its tests.

@somtochiama somtochiama force-pushed the trailing-slash branch 3 times, most recently from 7666490 to b40e5b6 Compare June 29, 2022 09:32
Signed-off-by: Somtochi Onyekwere <[email protected]>
Copy link
Contributor

@darkowlzz darkowlzz left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks.

@darkowlzz darkowlzz merged commit baf7988 into fluxcd:main Jun 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/helm Helm related issues and pull requests area/oci OCI related issues and pull requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

flux cli OCI helm repo reconcile panic
4 participants