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

Redirect from alias to canonical URL [azuredevops vso] #2939

Merged
merged 8 commits into from
Feb 8, 2019

Conversation

paulmelnikow
Copy link
Member

As described in #2340, this provides a way to replace an old alias with a redirect. This makes it easier to migrate our URLs over time without making our matching patterns more complicated.

The 301 redirect is sent back to the requester. If a user pastes the aliased URL into the address bar, it'll be replaced in the browser with the new URL, which gently encourages them to migrate.

We can do some follow-ons to migrate a few more of these.

Close #2340

As described in #2340, this provides a way to replace an old alias with a redirect. This makes it easier to migrate our URLs over time without making our matching patterns more complicated.

The 301 redirect is sent back to the requester. If a user pastes the aliased URL into the address bar, it'll be replaced in the browser with the new URL, which gently encourages them to migrate.

We can do some follow-ons to migrate a few more of these.

Close #2340
@paulmelnikow paulmelnikow added service-badge New or updated service badge core Server, BaseService, GitHub auth, Shared helpers labels Feb 6, 2019
@shields-ci
Copy link

shields-ci commented Feb 6, 2019

Warnings
⚠️

Found 'assert' statement added in core/base-service/redirector.js.
Please ensure tests are written using Chai expect syntax

Messages
📖 ✨ Thanks for your contribution to Shields, @paulmelnikow!

Generated by 🚫 dangerJS against 735ffb3

@paulmelnikow paulmelnikow added the blocker PRs and epics which block other work label Feb 6, 2019
@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-2939 February 6, 2019 03:01 Inactive
@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-2939 February 6, 2019 03:19 Inactive
@calebcartwright
Copy link
Member

Haven't gone through this yet, but I'm pumped about the feature 💯

@calebcartwright calebcartwright self-requested a review February 6, 2019 19:19
@calebcartwright
Copy link
Member

Self-requesting a review to remind me to look at this one tonight 😄

}

static get isDeprecated() {
return true
Copy link
Member

Choose a reason for hiding this comment

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

Will we need to register all the redirected services in the deprecated services list?

Copy link
Member Author

Choose a reason for hiding this comment

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

That list doesn't do anything. Those dates are useful to have readable in code, because it could let us write tests that remind us to downgrade or remove them. Though I think the dates could be placed in the call to deprecatedService as easily.

Copy link
Member

Choose a reason for hiding this comment

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

Would it be worth writing down redirected services somewhere in a similar fashion to keep track of them?

Copy link
Member Author

Choose a reason for hiding this comment

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

I suppose we could stop supporting the old routes after a good while. In that case having the date we started redirecting would be helpful.

I'd suggest we do that inline by throwing another parameter in the function call.

Copy link
Member

@calebcartwright calebcartwright Feb 7, 2019

Choose a reason for hiding this comment

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

I'm good to approve this, can wait if we wanna do that though

Went ahead and approved, but will wait to merge in case we do want to include dates as part of this

Copy link
Member Author

Choose a reason for hiding this comment

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

Have just added dates.

Copy link
Member

@calebcartwright calebcartwright left a comment

Choose a reason for hiding this comment

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

Looks good! Added a few thoughts inline

@calebcartwright
Copy link
Member

As a follow up item we should probably add documentation for redirects somewhere

@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-2939 February 7, 2019 17:34 Inactive
@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-2939 February 7, 2019 17:43 Inactive
calebcartwright
calebcartwright previously approved these changes Feb 8, 2019
@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-2939 February 8, 2019 05:22 Inactive
@shields-deployment
Copy link

This pull request was merged to master branch. This change is now waiting for deployment, which will usually happen within a few days. Stay tuned by joining our #ops channel on Discord!

After deployment, changes are copied to gh-pages branch:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocker PRs and epics which block other work core Server, BaseService, GitHub auth, Shared helpers service-badge New or updated service badge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants