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.

Sign up for GitHub

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

Jump to bottom

fix(#4916): Improve component resolver #4919

Merged
merged 1 commit into from
Nov 16, 2023
Merged

fix(#4916): Improve component resolver #4919

merged 1 commit into from
Nov 16, 2023

Conversation

christophd
Copy link
Contributor

  • Support URL query parameters when resolving components by given scheme
  • Do not resolve URLs that use parameter placeholder as a scheme
  • Properly extract scheme from URL that has query parameters

Fixes #4916

Release Note

fix: Improve component resolver

@github-actions GitHub Actions
Copy link
Contributor

🐫 Thank you for contributing!

Code Coverage Report ✔️ - Coverage changed: 33.5% --> 33.6% (Coverage difference: +.1%)

@christophd
Copy link
Contributor Author

@gansheer @squakez @claudio4j any idea why the validate CI job could be failing?

@squakez
Copy link
Contributor

squakez commented Nov 15, 2023

@gansheer @squakez @claudio4j any idea why the validate CI job could be failing?

" Error: File is not goimports-ed (goimports)"

Checkstyles formatting I guess.

@gansheer gansheer mentioned this pull request Nov 15, 2023
Merged
@claudio4j
Copy link
Contributor

In the prepare sections there are many /usr/bin/tar: ../../../go/pkg/mod/go.uber.org/multierr@v1.11.0/.gitignore: Cannot open: File exists errors. I restarted the checks.

@christophd
Copy link
Contributor Author

@squakez I do not have the Error: File is not goimports-ed (goimports) locally and the changes in that PR did not touch any imports 🤷

@gansheer
Copy link
Contributor

@squakez I do not have the Error: File is not goimports-ed (goimports) locally and the changes in that PR did not touch any imports 🤷

The error is not from your PR. The validate ci workflow is executed only on limited folder (pkg), so the PR that made the change was merged before on main. Since you are triggering the validate ci workflow you are the first seeing this error, but the source of validation error is very much present on main branch.

This PR will fix your issue after its merge.

@christophd
fix(apache#4916): Improve component resolver
00c7731 
- Support URL query parameters when resolving components by given scheme
- Do not resolve URLs that use parameter placeholder as a scheme
- Properly extract scheme from URL that has query parameters
@christophd christophd force-pushed the issue/4916/component-resolve-regression branch from 9bc48ff to 00c7731 Compare November 15, 2023 21:20
@christophd
Copy link
Contributor Author

@gansheer many thanks! I have merged your PR and rebased this one so lets see those CI jobs becoming green now

@github-actions GitHub Actions
Copy link
Contributor

🐫 Thank you for contributing!

Code Coverage Report ✔️ - Coverage changed: 33.5% --> 33.6% (Coverage difference: +.1%)

@christophd christophd merged commit b3464f0 into apache:main Nov 16, 2023
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oscerd oscerd oscerd approved these changes

@gansheer gansheer gansheer approved these changes

@squakez squakez squakez approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Component resolvable variables regression
5 participants
@christophd @squakez @claudio4j @gansheer @oscerd