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

fix: updated baseHRefRegex to perform lazy match #9724

Merged
merged 2 commits into from Jun 20, 2022

Conversation

gdsoumya
Copy link
Member

Signed-off-by: Soumya Ghosh Dastidar gdsoumya@gmail.com

Fixes #9660
Fixes #9692

This PR fixes the baseHRefRegex to perform a lazy match preventing index.html from being broken

After change resulting index.html (--rootpath=/argocd):

<!doctype html>
<html lang="en">
<head>
    <meta charset="UTF-8">
    <title>Argo CD</title>
    <base href="/argocd/">
    <meta name="viewport" content="width=device-width,initial-scale=1">
    <link rel="icon" type="image/png" href="assets/favicon/favicon-32x32.png" sizes="32x32" />
    <link rel="icon" type="image/png" href="assets/favicon/favicon-16x16.png" sizes="16x16" />
    <link href="assets/fonts.css" rel="stylesheet">
    <script defer="defer" src="main.ee4eaaa358b34122032e.js"></script>
</head>
<body>
    <noscript>
        <p>Your browser does not support JavaScript. Please enable JavaScript to view the site. Alternatively, Argo CD
            can be used with the <a href="https://argoproj.github.io/argo-cd/cli_installation/">Argo CD CLI</a>.</p>
    </noscript>
    <div id="app"></div>
</body>
</html>

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
  • The title of the PR states what changed and the related issues number (used for the release note).
  • I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
  • I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
  • Does this PR require documentation updates?
  • I've updated documentation as required by this PR.
  • Optional. My organization is added to USERS.md.
  • I have signed off all my commits as required by DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My build is green (troubleshooting builds).

Signed-off-by: Soumya Ghosh Dastidar <gdsoumya@gmail.com>
@alexmt alexmt requested a review from crenshaw-dev June 20, 2022 17:59
@codecov
Copy link

codecov bot commented Jun 20, 2022

Codecov Report

Merging #9724 (cc5934e) into master (a67b97d) will increase coverage by 0.00%.
The diff coverage is 50.00%.

@@           Coverage Diff           @@
##           master    #9724   +/-   ##
=======================================
  Coverage   45.78%   45.79%           
=======================================
  Files         226      226           
  Lines       26714    26715    +1     
=======================================
+ Hits        12232    12233    +1     
  Misses      12818    12818           
  Partials     1664     1664           
Impacted Files Coverage Δ
server/server.go 54.65% <50.00%> (+0.07%) ⬆️
util/settings/settings.go 48.16% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a67b97d...cc5934e. Read the comment docs.

@@ -132,7 +132,7 @@ var backoff = wait.Backoff{

var (
clientConstraint = fmt.Sprintf(">= %s", common.MinClientVersion)
baseHRefRegex = regexp.MustCompile(`<base href="(.*)">`)
baseHRefRegex = regexp.MustCompile(`<base href="(.*?)">`)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you please add a test as well? I would suggest moving the line below into function and add a unit test for it

s.indexData = []byte(baseHRefRegex.ReplaceAllString(string(data), fmt.Sprintf(`<base href="/%s/">`, strings.Trim(s.BaseHRef, "/"))))

Copy link
Member Author

Choose a reason for hiding this comment

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

added unit test

@alexmt
Copy link
Collaborator

alexmt commented Jun 20, 2022

I think this is a regression, so adding cherry-pick label cc @crenshaw-dev , @leoluz

@alexmt alexmt added the cherry-pick/2.4 Candidate for cherry picking into the 2.4 release branch label Jun 20, 2022
Signed-off-by: Soumya Ghosh Dastidar <gdsoumya@gmail.com>
Copy link
Collaborator

@alexmt alexmt left a comment

Choose a reason for hiding this comment

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

Thank you!

@alexmt alexmt merged commit 952230e into argoproj:master Jun 20, 2022
alexmt pushed a commit that referenced this pull request Jun 21, 2022
* fix: updated baseHRefRegex to perform lazy match

Signed-off-by: Soumya Ghosh Dastidar <gdsoumya@gmail.com>

* feat: added unit tests for replaceBaseHRef

Signed-off-by: Soumya Ghosh Dastidar <gdsoumya@gmail.com>
@crenshaw-dev
Copy link
Collaborator

Alex cherry-picked onto 2.4 for release with 2.4.2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick/2.4 Candidate for cherry picking into the 2.4 release branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ArgoCD 2.4.0 webUI with rootpath no longer works. ArgoCD UI: Wrong index.html when running on subpath
3 participants