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.

Sign up for GitHub

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

Jump to bottom

Integrate new path-to-regexp with trailing optionals [appveyor azuredevops npm gitlab date githubmanifest githubpackage jenkins matrix readthedocs requires shippable snyk wordpress] #2644

Merged
merged 15 commits into from
Jan 8, 2019
Merged

Integrate new path-to-regexp with trailing optionals [appveyor azuredevops npm gitlab date githubmanifest githubpackage jenkins matrix readthedocs requires shippable snyk wordpress] #2644

merged 15 commits into from
Jan 8, 2019

Conversation

paulmelnikow
Copy link
Member

@paulmelnikow paulmelnikow commented Jan 6, 2019 •
edited
Loading

@paulmelnikow paulmelnikow added core Server, BaseService, GitHub auth, Shared helpers dependencies Related to dependency updates labels Jan 6, 2019
@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-2644 January 6, 2019 00:11 Inactive
@shields-ci
Copy link

shields-ci commented Jan 6, 2019 •
edited
Loading

Warnings
⚠️ This PR modified service code for appveyor but not its test code.
That's okay so long as it's refactoring existing code.
⚠️ This PR modified service code for date but not its test code.
That's okay so long as it's refactoring existing code.
⚠️ This PR modified service code for github but not its test code.
That's okay so long as it's refactoring existing code.
⚠️ This PR modified service code for gitlab but not its test code.
That's okay so long as it's refactoring existing code.
⚠️ This PR modified service code for jenkins but not its test code.
That's okay so long as it's refactoring existing code.
⚠️ This PR modified service code for azure-devops but not its test code.
That's okay so long as it's refactoring existing code.
⚠️ This PR modified service code for readthedocs but not its test code.
That's okay so long as it's refactoring existing code.
⚠️ This PR modified service code for requires but not its test code.
That's okay so long as it's refactoring existing code.
⚠️ This PR modified service code for shippable but not its test code.
That's okay so long as it's refactoring existing code.
⚠️ This PR modified service code for snyk but not its test code.
That's okay so long as it's refactoring existing code.
⚠️ This PR modified service code for wordpress but not its test code.
That's okay so long as it's refactoring existing code.
⚠️ This PR modified service code for matrix but not its test code.
That's okay so long as it's refactoring existing code.
Messages
📖 ✨ Thanks for your contribution to Shields, @paulmelnikow!
📖

Thanks for contributing to our documentation. We ❤️ our documentarians!

Generated by 🚫 dangerJS against 053d028

@paulmelnikow paulmelnikow changed the title [wip] Try to integrate new path-to-regexp with trailing optionals [appveyor azuredevops npm] [wip] Try to integrate new path-to-regexp with trailing optionals [appveyor azuredevops npm gitlab] Jan 6, 2019
@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-2644 January 6, 2019 00:14 Inactive
@paulmelnikow paulmelnikow mentioned this pull request Jan 6, 2019
Merged
@paulmelnikow paulmelnikow changed the title [wip] Try to integrate new path-to-regexp with trailing optionals [appveyor azuredevops npm gitlab] [wip] Try to integrate new path-to-regexp with trailing optionals [appveyor azuredevops npm gitlab date githubmanifest githubpackage jenkins matrix readthedocs requires shippable snyk wordpress] Jan 6, 2019
@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-2644 January 6, 2019 00:30 Inactive
@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-2644 January 6, 2019 00:31 Inactive
@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-2644 January 6, 2019 01:23 Inactive
@paulmelnikow
Copy link
Member Author

How do folks feel about merging this with the github link in place while we wait for a published version? I don't love the idea, but if it's temporary I don't mind too much.

@paulmelnikow paulmelnikow changed the title [wip] Try to integrate new path-to-regexp with trailing optionals [appveyor azuredevops npm gitlab date githubmanifest githubpackage jenkins matrix readthedocs requires shippable snyk wordpress] Integrate new path-to-regexp with trailing optionals [appveyor azuredevops npm gitlab date githubmanifest githubpackage jenkins matrix readthedocs requires shippable snyk wordpress] Jan 6, 2019
@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-2644 January 6, 2019 13:27 Inactive
@calebcartwright
Copy link
Member

How do folks feel about merging this with the github link in place while we wait for a published version?

Generally speaking I avoid doing so, but agree that if this is temporary (seems like that should be merged and published soon) then it is probably okay, especially considering the benefits of the trailing optional

@paulmelnikow paulmelnikow added the blocker PRs and epics which block other work label Jan 6, 2019
calebcartwright
calebcartwright reviewed Jan 6, 2019
View reviewed changes
services/github/github-manifest.service.js Outdated
@@ -25,8 +25,7 @@ class GithubManifestVersion extends ConditionalGithubAuthService {
static get route() {
return {
base: 'github/manifest-json/v',
format: '([^/]+)/([^/]+)/?([^/]+)?',
capture: ['user', 'repo', 'branch'],
pattern: ':user/:repo/:branch?',
Copy link
Member

Choose a reason for hiding this comment

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

Sidenote, I'm seeing an invalid response data badge message for the example badge used here. Seeing this in prod too so not related to this PR.

https://img.shields.io/github/manifest-json/v/RedSparr0w/IndieGala-Helper.svg

calebcartwright
calebcartwright reviewed Jan 6, 2019
View reviewed changes
services/github/github-package-json.service.js Outdated
@@ -23,8 +23,7 @@ class GithubPackageJsonVersion extends ConditionalGithubAuthService {
static get route() {
return {
base: 'github/package-json/v',
format: '([^/]+)/([^/]+)/?([^/]+)?',
capture: ['user', 'repo', 'branch'],
pattern: ':user/:repo/:branch?',
Copy link
Member

Choose a reason for hiding this comment

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

Ditto on example error here

Copy link
Member Author

Choose a reason for hiding this comment

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

Did you mean to add something like (branch) to the title? I've just gone ahead and done that.

Copy link
Member

@calebcartwright calebcartwright Jan 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry I should've been more clear. I meant that this badge is showing an invalid response data error with the example too (also occurs in prod so not related to these changes)

calebcartwright
calebcartwright reviewed Jan 6, 2019
View reviewed changes
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! Inline question below on wordpress service, and a couple other thoughts not directly related to these changes

services/wordpress/wordpress-platform.service.js Show resolved Hide resolved
@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-2644 January 7, 2019 05:06 Inactive
@paulmelnikow paulmelnikow mentioned this pull request Jan 8, 2019
Closed
calebcartwright
calebcartwright previously approved these changes Jan 8, 2019
View reviewed changes
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.

Think this is good to go! I may open a separate issue to deal with those erorring examples

@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-2644 January 8, 2019 05:03 Inactive
@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-2644 January 8, 2019 05:03 Inactive
@paulmelnikow
Copy link
Member Author

Could you re-up this @calebcartwright? Every time a merge conflict has to be resolved manually the review gets dismissed.

@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-2644 January 8, 2019 21:54 Inactive
@paulmelnikow paulmelnikow mentioned this pull request Jan 8, 2019
Merged
@paulmelnikow paulmelnikow merged commit d1c5378 into master Jan 8, 2019
@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:

@paulmelnikow paulmelnikow deleted the path-to-regexp-update-2 branch January 8, 2019 23:50
@paulmelnikow paulmelnikow mentioned this pull request Jan 9, 2019
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@calebcartwright calebcartwright calebcartwright approved these changes

Assignees
No one assigned
Labels
blocker PRs and epics which block other work core Server, BaseService, GitHub auth, Shared helpers dependencies Related to dependency updates
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@paulmelnikow @shields-ci @calebcartwright