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

build(cypress)!: revise Cypress base URL value for staging scenario #363

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

andygout
Copy link
Contributor

@andygout andygout commented Mar 10, 2023

Description

This PR changes the value assigned to cypressEnv.CYPRESS_BASE_URL in the staging scenario so that the the environment variable contains the entire URL rather than just the hostname.

The rationale is that this will require a new key type added to cp-vault-interface for heroku-url (done in this PR). The other xxxxx-url validators in cp-vault-interface (currently only raven-url and slack-webhook-url) use the URL validator.

From a consistency perspective it would make sense that all URL values in Vault are the full URL rather than just the hostname.

We can also validate a) that the value is a URL, b) that its protocol is https:, and c) its format matches the Heroku hostname format, as long as the value is updated using cp-vault-interface rather than manually, though admittedly this is dependent on individuals and therefore not a guarantee while the hardcoded https: in the Cypress plugin's existing state does enforce the protocol, so there is certainly an argument as to whether this is the best approach (both here and more generally).

I have added @Financial-Times/cp-reliability as reviewers to this PR as I think it feeds into this larger discussion of secret management.

Naming change for the environment variable

I changed the name from CY_ to CYPRESS_ so as to make explicit the purpose of the environment variable as those less familiar with Cypress may not be aware of its cy usage shorthand.

Semver release decision

I have used a ! in the commit to indicate a breaking change. While I don't think there are any apps that have yet migrated to Tool Kit and are running end-to-end Cypress tests against the staging app in their CircleCI production deployment pipeline, I'm not certain this is definitely the case and so in the interests of caution it should be a major release as the tool-kit/e2e-test-staging job will fail at Run smoke tests on staging without the necessary environment variable amends.

Checklist:

  • My branch has been rebased onto the latest commit on main (don't merge main into your branch)
  • My commit messages are conventional commits, for example: feat(circleci): add support for nightly workflows, fix: set Heroku app name for staging apps too

@andygout andygout requested a review from a team March 10, 2023 11:22
@andygout andygout requested a review from a team as a code owner March 10, 2023 11:22
@andygout andygout force-pushed the build-cypress-plugin-revise-base-url-for-staging branch from 0c4cbe1 to 2c99904 Compare March 10, 2023 13:23
@andygout andygout changed the title build!: revise Cypress base URL value for staging scenario build(cypress)!: revise Cypress base URL value for staging scenario Mar 10, 2023
@andygout
Copy link
Contributor Author

Update: The cp-vault-interface PR was closed for reasons described by the discussion (non-secret values are not subject to validation), meaning this PR comes down to the points of:

  • consistency of having full URLs in Vault (i.e. including the protocol)
  • explicitness of the env var name

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.

1 participant