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

Closes issue #2357: Update status url to point to either dev or prod status service url #2432

Merged
merged 3 commits into from
Nov 2, 2021

Conversation

HyperTHD
Copy link
Contributor

@HyperTHD HyperTHD commented Nov 1, 2021

Issue This PR Addresses

This PR closes #2357

Type of Change

  • Bugfix: Change which fixes an issue
  • New Feature: Change which adds functionality
  • Documentation Update: Change which improves documentation
  • UI: Change which improves UI

Description

The current status icon pointed to the production url for the status service. This is due to it being hard-coded. This PR addresses that by updating our docker config, next config, and the status icon's href to point to the correct status service url based on environment settings.

NOTE: This might not work immediately on the vercel views because they need to be updated as well. cc @humphd if this doesn't work off the bat.

Checklist

  • Quality: This PR builds and passes our npm test and works locally
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not (if applicable)
  • Documentation: This PR includes updated/added documentation to user exposed functionality or configuration variables are added/changed or an explanation of why it does not(if applicable)

@gitpod-io
Copy link

gitpod-io bot commented Nov 1, 2021

Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

I was literally thinking about the need for this earlier today, and here you go and fix it! Awesome!

I need to add this URL to the Vercel env manually, which I'll do now.

docker/production.yml Outdated Show resolved Hide resolved
@humphd
Copy link
Contributor

humphd commented Nov 1, 2021

Add STATUS_URL with value https://dev.api.telescope.cdot.systems/v1/status to Vercel Telescope config.

@HyperTHD
Copy link
Contributor Author

HyperTHD commented Nov 1, 2021

Add STATUS_URL with value https://dev.api.telescope.cdot.systems/v1/status to Vercel Telescope config.

Would this file be the vercel.json file in the web folder? Confused about this

@humphd
Copy link
Contributor

humphd commented Nov 1, 2021

Sorry, "I added..." I have to do it manually, it's done.

humphd
humphd previously requested changes Nov 2, 2021
docker/production.yml Outdated Show resolved Hide resolved
docker/docker-compose.yml Show resolved Hide resolved
@HyperTHD
Copy link
Contributor Author

HyperTHD commented Nov 2, 2021

Apologizes for the late fix, vercel changes means this PR will probably have 1 failing test no matter what if I'm understanding this right

Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

I'm not clear why Vercel keeps needing authorization for this STATUS_URL variable. I've tweeted them to ask if I'm doing it wrong (I did it the same as we usually do).

@@ -52,6 +52,7 @@ services:
- WEB_URL
- SEARCH_URL
- FEED_DISCOVERY_URL
- STATUS_URL
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought @manekenpix said to remove this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I confused that with another declaration above, my mistake. Gonna clear this rn

@humphd
Copy link
Contributor

humphd commented Nov 2, 2021

Based on https://twitter.com/leeerob/status/1455553924119740421 I've disable git fork protection, so hopefully the next PR/push will not need Vercel authorization.

@HyperTHD HyperTHD merged commit e1675d3 into Seneca-CDOT:master Nov 2, 2021
@HyperTHD HyperTHD deleted the issue-2357 branch November 2, 2021 16:19
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.

Show correct dev/prod status info on front-end
3 participants