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

Refactor [SymfonyInsight] to new service model and rename #2572

Merged
merged 16 commits into from
Jan 7, 2019

Conversation

calebcartwright
Copy link
Member

@calebcartwright calebcartwright commented Dec 21, 2018

Based on some discussion/feedback here, this PR now contains several changes:

  • Renames the Sensiolabs badge/service content to SymfonyInsight to reflect the rebranding of that product/service
  • Refactors the original service to the new service model (using BaseXmlService)
  • Updates the color scheme of the original/initial badge type (SymfonyInsight Grade) to more closely mirror the colors used by the vendor/service provider
  • Adds a new badge type (violation counts/summary)
  • Adds both mocked and live tests (there were none before) for both the grade & violation badges using the new path symfony/i as well as a couple tests for the old path sensiolabs/i to check for backwards compatibility

Refs #1358

@calebcartwright calebcartwright added the service-badge New or updated service badge label Dec 21, 2018
@shields-ci
Copy link

shields-ci commented Dec 21, 2018

Warnings
⚠️

📚 Remember to ensure any changes to serverSecrets in services/symfony/symfony-test-helpers.js are reflected in the server secrets documentation

⚠️

📚 Remember to ensure any changes to serverSecrets in services/symfony/symfony-insight.service.js are reflected in the server secrets documentation

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

Generated by 🚫 dangerJS against b42d572

@paulmelnikow
Copy link
Member

Thanks for picking this up :) As far as testing strategy, Wheelmap is a similar case that @PyvesB recently did some work on in #2486. It's a combination of mocked tests and live tests, with live tests that run when a token is available. We created accounts with tokens for prod, CI, and staging.

I forgot that document existed; just opened a PR to bring a couple things up to date.

@calebcartwright
Copy link
Member Author

Excellent, I was just looking through the wheelmap tests and think I can get that working here. I assume that means that the tokens are already set up for Sensiolabs in CI too?

@paulmelnikow
Copy link
Member

They are not, though we probably should add one. All the CI tokens are referenced in the CircleCI config. Probably we should also collect these secrets somewhere.

@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-2572 December 21, 2018 20:58 Inactive
@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-2572 December 23, 2018 03:35 Inactive
@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-2572 December 24, 2018 05:20 Inactive
@calebcartwright calebcartwright changed the title [WIP] Migrate [Sensiolabs] to new service model Refactor (and rename) [SymfonyInsight] to new service model Dec 24, 2018
@calebcartwright calebcartwright changed the title Refactor (and rename) [SymfonyInsight] to new service model Refactor [SymfonyInsight] to new service model and rename Dec 24, 2018
@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-2572 December 24, 2018 05:34 Inactive
@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-2572 December 24, 2018 05:37 Inactive
@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-2572 January 2, 2019 02:15 Inactive
@paulmelnikow
Copy link
Member

From the review app:

screen shot 2019-01-01 at 9 45 23 pm

Are these previews wrong? I feel like the LHS on the first one probably should be grade.

Colors look good!

These example links are both showing not authorized to access project:

@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-2572 January 2, 2019 04:13 Inactive
@calebcartwright
Copy link
Member Author

We can use the tokens I created for building out the tests, although honestly any API/User token will do as those are public analyses/projects

@paulmelnikow
Copy link
Member

Do you think we should change the error message when a token is not configured?

@calebcartwright
Copy link
Member Author

calebcartwright commented Jan 2, 2019

Interesting idea. What exactly were you envisioning? Throwing some kind of error when the tokens are missing, or making the request anyway and adding a new/different error messages?

I could see that being helpful for us when looking at staging/prod, and also for any potential self-hosting Shields users that are trying to create badges, especially from their private SymfonyInsight analyses. I definitely think it is worth making a distinction between "oops, you have no tokens" vs. "well you have a token but that token isn't authorized to access project foo"

@paulmelnikow
Copy link
Member

Yea, I'm thinking throwing an error when there are no tokens, perhaps a new subclass ImproperlyConfigured.

@calebcartwright
Copy link
Member Author

I really like that idea. Are you good with updating this PR with a throw, and doing the subclass work in a separate stream?

@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-2572 January 3, 2019 01:04 Inactive
@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-2572 January 3, 2019 01:05 Inactive
@calebcartwright
Copy link
Member Author

calebcartwright commented Jan 3, 2019

Alright the service will now throw an error (I chose Inaccessible as it seemed the best fit with current error options) when the tokens are missing. This had an interesting impact on the tests. There's now one test that validates the error is thrown when the tokens are missing, but all the other tests need to have auth present.

I've added a review comment on the relevant portions of the test file on that topic

@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-2572 January 3, 2019 18:55 Inactive
@paulmelnikow paulmelnikow temporarily deployed to shields-staging-pr-2572 January 3, 2019 19:01 Inactive
@paulmelnikow paulmelnikow mentioned this pull request Jan 4, 2019
@calebcartwright
Copy link
Member Author

I believe this is ready to merge (barring any final comments/suggestions), but there's now a merge conflict that GH is telling me I don't have access to resolve 🤔

I deleted the sensiolabs.service.js file as part of the refactor & rename, and the service content was moved to symfony related files.

@paulmelnikow
Copy link
Member

paulmelnikow commented Jan 7, 2019

Have you tried resolving it in the CLI? If you merge master into this branch you should be able to.

Added: I took care of it :)

@paulmelnikow paulmelnikow merged commit ca487ae into master Jan 7, 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 sensiolabs-refactor branch January 7, 2019 05:28
@calebcartwright
Copy link
Member Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
service-badge New or updated service badge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants