Skip to content
This repository was archived by the owner on Nov 29, 2023. It is now read-only.

Conversation

@brianluisgomez
Copy link
Contributor

⚠️Please do not make any changes to spec files in this repository!⚠️

All spec changes should be done in the api-specs repository.

Please follow the Guide for contributing to our developer documentation.

@cypress
Copy link

cypress bot commented Dec 2, 2022



Test summary

230 0 0 0Flakiness 0


Run details

Project api-docs
Status Passed
Commit 55d2d97
Started Dec 20, 2022 12:18 PM
Ended Dec 20, 2022 12:24 PM
Duration 06:29 💡
OS Linux Ubuntu - 22.04
Browser Chrome 108

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@github-actions
Copy link
Contributor

github-actions bot commented Dec 6, 2022

@github-actions
Copy link
Contributor

github-actions bot commented Dec 6, 2022

@github-actions
Copy link
Contributor

github-actions bot commented Dec 6, 2022

@brianluisgomez brianluisgomez marked this pull request as ready for review December 6, 2022 14:54
@brianluisgomez brianluisgomez requested a review from a team December 6, 2022 14:54
@github-actions
Copy link
Contributor

github-actions bot commented Dec 9, 2022

ckoegel
ckoegel previously approved these changes Dec 9, 2022
@github-actions
Copy link
Contributor

Co-authored-by: AJ Rice <53190766+ajrice6713@users.noreply.github.com>
@github-actions
Copy link
Contributor

export const downloadButtonTester = (path) => {
it('checks the download button to verify it exists',() => {
cy.visit(`/apis/${path}`)
cy.visit(`/apis/${path}/`)

Choose a reason for hiding this comment

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

Please explain the trailing slash / updates here and elsewhere - what happens without change(logic in lambda?), what have you tried to resolve it besides this change ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The /s needed to be updated like this because of the way that the lambda pulls out the subdomain and puts it in the path for the s3 bucket. Without changing this here I was getting errors and a strange URI in the links but this small change makes everything work correctly in the new URL

Choose a reason for hiding this comment

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

So can't we put a slash when it's missing at the end of URL in respective lambdas before custom logic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could but it seemed like extra work in there that was unnecessary. Currently lambda is taking the subdomain and adding it in the path as the entrypoint. There's nothing defined to handle the end of any particular URI so there's more logic that would need to thought-through and added to get that piece also working on each call. Since every endpoint needs to be added with or without a slash in our site source code anyway, the simpler solution seems to me to just include it on the paths when we define them instead of adding another manipulation of the URI in the lambda.

That was my thinking at least, if it's more correct/better to define that logic in the lambda and revert these let me know and I'll do that 👍

matthewkmartin
matthewkmartin previously approved these changes Dec 15, 2022
@github-actions
Copy link
Contributor

@github-actions
Copy link
Contributor

@brianluisgomez brianluisgomez merged commit 9dddc3f into main Dec 20, 2022
@brianluisgomez brianluisgomez deleted the DX-3003 branch December 20, 2022 12:17
@github-actions
Copy link
Contributor

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants