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 Incorrect ArgoCD and Gitea UI URLs in README.md #267

Merged
merged 4 commits into from
May 29, 2024

Conversation

SupriyaNallapeta
Copy link
Contributor

@SupriyaNallapeta SupriyaNallapeta commented May 29, 2024

This pull request corrects the URLs for accessing the ArgoCD and Gitea UIs specified in the README. The previous URLs did not correctly reflect the actual URLs for accessing these services.

Changes made:

These changes ensure that users are directed to the correct locations when setting up their local development environment.

@SupriyaNallapeta SupriyaNallapeta force-pushed the main branch 3 times, most recently from 0c7fc17 to 221db2f Compare May 29, 2024 08:06
Copy link
Collaborator

@nabuskey nabuskey 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 very much for this. Definitely an oversight on our part. I don't think we need to make changes for the codespaces section since codespaces provides a single domain name only.

README.md Outdated
Comment on lines 231 to 232
* ArgoCD: `echo https://${CODESPACE_NAME}-8080.${GITHUB_CODESPACES_PORT_FORWARDING_DOMAIN}/argocd`
* Gitea: `echo https://${CODESPACE_NAME}-8080.${GITHUB_CODESPACES_PORT_FORWARDING_DOMAIN}/gitea`
* ArgoCD: `echo https://argocd.${CODESPACE_NAME}-8080.${GITHUB_CODESPACES_PORT_FORWARDING_DOMAIN}`
* Gitea: `echo https://gitea.${CODESPACE_NAME}-8080.${GITHUB_CODESPACES_PORT_FORWARDING_DOMAIN}`
Copy link
Collaborator

Choose a reason for hiding this comment

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

This bit does not need to be changed because in Codespaces, we have to use path based routing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like it does need this change.
I ran it is codespace which gave me this invalid link http://psychic-space-potato-gxq4q6pq4ppfwwxx-8080.app.github.dev:8080/argocd at the end to access argocd.
But the same worked when I modified the link as http://argocd.psychic-space-potato-gxq4q6pq4ppfwwxx-8080.app.github.dev:8080
Screenshot 2024-05-29 at 9 49 47 AM

I added another commit to idpbuilder/pkg/cmd/create/root.go to fix this output url. Correct me if I'm missing anything here.

Copy link
Collaborator

@nabuskey nabuskey May 29, 2024

Choose a reason for hiding this comment

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

Thanks for the detailed info. I just tried it in Codespaces through my browser (Firefox) and it does work as expected. That is, I can access UIs at subpath (/argocd and /gitea) The command used was:

./idpbuilder create --protocol http  \
    --host ${CODESPACE_NAME}-8080.${GITHUB_CODESPACES_PORT_FORWARDING_DOMAIN} \
    --port 8080 --use-path-routing

I am surprised you can access argocd UI at argocd.<GITHUB_CODESPACES_PORT_FORWARDING_DOMAIN>. As far as I am aware, most browsers won't let you access it because of HSTS. Are you running this in your browser?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right! argocd.<GITHUB_CODESPACES_PORT_FORWARDING_DOMAIN> is not working due to HSTS in my browser. However, <GITHUB_CODESPACES_PORT_FORWARDING_DOMAIN>/argocd is giving 404. How are you accessing these UIs?

Copy link
Collaborator

Choose a reason for hiding this comment

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

https://${CODESPACE_NAME}-8080.${GITHUB_CODESPACES_PORT_FORWARDING_DOMAIN}/argocd

This should work. Note the port is the default HTTPS port 443 because this part is handled by Codespaces. My suggestion is to destroy everything and try it again.

# delete cluster. THIS WILL DELETE EVERYTHING IN CLUSTER
kind delete clusters localdev && docker system prune -f && docker volume prune -f
./idpbuilder create --protocol http --host ${CODESPACE_NAME}-8080.${GITHUB_CODESPACES_PORT_FORWARDING_DOMAIN}  --port 8080 --use-path-routing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://${CODESPACE_NAME}-8080.${GITHUB_CODESPACES_PORT_FORWARDING_DOMAIN}/argocd is working for me as well.
Previously, I was using the URL provided at the end of the idpbuilder create command output (check screenshot here), which had :8080 appended at the end. Looks like this is not needed with path based routing in Codespace. I have updated the PR to reflect this finding.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The changes to code has some unintended consequences. For example, if you are running it on your local machine with path based routing, the printed URL is incorrect:

Expected: https://cnoe.localtest.me:8443/argocd
Reality: https://cnoe.localtest.me/argocd

If you are running this in environment where it's fronted by another proxy like Codespaces, what you did is correct. But we can't assume we are fronted by another proxy whenever the --use-path-routing flag is used. This flag is valid for local machine use cases as well.

I agree that the URL printed out at the end is incorrect for Codespaces and should be fixed. Proper fix for this will involve detecting if we have a proxy in front of us.

I think we can merge this PR with changes to the README file only. We can tackle the codespaces problem in another PR and issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed! I din't think of that scenario. removed those changes from this PR

Copy link
Collaborator

@nabuskey nabuskey left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you very much for this PR!!

@nabuskey nabuskey merged commit 08a1af6 into cnoe-io:main May 29, 2024
1 check passed
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.

2 participants