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

Issue 928 #934

Merged
merged 3 commits into from
Jan 30, 2025
Merged

Issue 928 #934

merged 3 commits into from
Jan 30, 2025

Conversation

ernstvonoelsen
Copy link
Contributor

targets resolution of #928.

@@ -47,6 +47,12 @@ func NewSortedFilesFromPaths(paths []string, opts SymlinkAllowOpts) ([]*File, er

relativePath := ""
pathPieces := strings.Split(path, "=")
if strings.HasPrefix(pathPieces[0], "http://") || strings.HasPrefix(pathPieces[0], "https://") {
pathPieces = []string{path}
} else if len(pathPieces) > 1 && (strings.HasPrefix(pathPieces[1], "http://") || strings.HasPrefix(pathPieces[1], "https://")) {
Copy link
Member

Choose a reason for hiding this comment

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

What is the scenario where this if statement is triggered?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@joaopapereira this is a safetynet for the use case that the file's path is overwritten on the command line; cf. the case 2 part in L60.

Example: want to cover both scenarios of

  • path = "myOverwrittenPath=https://example.com?hello=world"
  • path = "https://example.com?hello=world"

Copy link
Member

@joaopapereira joaopapereira left a comment

Choose a reason for hiding this comment

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

Sorry for dropping this out, do you mind adding some tests for this? just 1 or 2 basic tests I think it would be enough

@ernstvonoelsen
Copy link
Contributor Author

ernstvonoelsen commented Jan 29, 2025

@joaopapereira done ✔️

for an e2e test, cf. my comment in the issue

Copy link
Member

@joaopapereira joaopapereira left a comment

Choose a reason for hiding this comment

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

LGTM

@joaopapereira joaopapereira merged commit faba705 into carvel-dev:develop Jan 30, 2025
5 checks passed
@ernstvonoelsen ernstvonoelsen deleted the issue-928 branch January 30, 2025 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

2 participants