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

added sentry webpack plugin #5118

Merged
merged 1 commit into from
Dec 14, 2021

Conversation

Wassaf-Shahzad
Copy link
Contributor

@Wassaf-Shahzad Wassaf-Shahzad commented Nov 15, 2021

Pre-Flight checklist

What are the relevant tickets?

Closes #5117

What's this PR do?

This PR adds a sentry Webpack plugin do add source map to sentry.

How should this be manually tested?

Source map should be uploaded in sentry.

Introduces a new settings in CI

This PR adds the following environment variables
- ORG_NAME ( organisation name as defined in sentry)
- PROJECT_NAME (name of the project as defined in sentry)
- SENTRY_RELEASE (same as the one passed to sentry init call in sentry client)
- SENTRY_AUTH_TOKEN (sentry auth token with project:write scope)

@codecov-commenter
Copy link

codecov-commenter commented Nov 16, 2021

Codecov Report

Merging #5118 (2e7292c) into master (164e2b4) will decrease coverage by 0.00%.
The diff coverage is 83.33%.

❗ Current head 2e7292c differs from pull request most recent head ead5720. Consider uploading reports for the commit ead5720 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5118      +/-   ##
==========================================
- Coverage   93.99%   93.99%   -0.01%     
==========================================
  Files         497      497              
  Lines       23048    23052       +4     
  Branches      943     1409     +466     
==========================================
+ Hits        21664    21667       +3     
  Misses       1282     1282              
- Partials      102      103       +1     
Impacted Files Coverage Δ
micromasters/settings.py 89.50% <83.33%> (-0.30%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 164e2b4...ead5720. Read the comment docs.

@odlbot odlbot had a problem deploying to micromasters-ci-pr-5118 November 17, 2021 16:31 Failure
@odlbot odlbot had a problem deploying to micromasters-ci-pr-5118 November 17, 2021 18:11 Failure
@Wassaf-Shahzad Wassaf-Shahzad force-pushed the wassaf/5106-add-sentry-webpack-plugin branch from b4097dd to d545b99 Compare November 17, 2021 18:22
@odlbot odlbot had a problem deploying to micromasters-ci-pr-5118 November 17, 2021 18:23 Failure
@Wassaf-Shahzad
Copy link
Contributor Author

Wassaf-Shahzad commented Nov 17, 2021

i have updated the webpack config and added some environment variables as well( I didn't chose a dot file as we could add the token in GitHub secrets).
I do need one config value from sentry (SENTRY_AUTH token). I can create that on a developer level but still thought I should wait for you go ahead. (The checks are failing due to the env variables which should be added to the workflow conf)

@Wassaf-Shahzad Wassaf-Shahzad force-pushed the wassaf/5106-add-sentry-webpack-plugin branch 2 times, most recently from c827c7b to a640a9b Compare November 24, 2021 08:36
@Wassaf-Shahzad
Copy link
Contributor Author

Made a small change in webpack config for prod. I made the sentry plugin conditional so as not to upload releases to sentry during CI builds.

Comment on lines 58 to 62
...process.env.IS_NOT_CI ? [new SentryWebpackPlugin({
authToken: process.env.SENTRY_AUTH_TOKEN,
org : process.env.ORG_NAME,
project : process.env.PROJECT_NAME,
release : process.env.SENTRY_RELEASE,
Copy link
Contributor

Choose a reason for hiding this comment

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

A few things here:

  • You should open a ticket with devops to set these values
  • Could you namespace them all under SENTRY_*?
  • Currently the release version comes from settings.py, there's no environment variable for that and we don't currently have any automation around setting this var either. So I think some work needs to go into pulling that out into a version file or otherwise getting it extracted somehow during the heroku build. The release bot expects it to live in settings.py though so the latter might be best for the time being.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have addressed the first two issues However I am not sure I understand you last point. Is there any related document or commit I could look in to

Copy link
Contributor

Choose a reason for hiding this comment

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

I've put up a fix to the release script here: mitodl/release-script#358, that will allow you to move the version into a VERSION file and then replace the current definition of VERSION in settings.py with a few lines of code that read that file's contents. Then do the same in webpack.config.prod.js and use that for the value of release.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, that PR is approved, but I'm going to hold off on merging it until after this merged, let me know when i can review this again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, i had addressed the feedback a while back but forgot tor re-request the review. Sorry for the delay

Copy link
Contributor

Choose a reason for hiding this comment

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

@Wassaf-Shahzad I'm not seeing those changes here, specifically the usage of a VERSION file.

Copy link
Contributor

@rhysyngsun rhysyngsun left a comment

Choose a reason for hiding this comment

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

LGTM

@Wassaf-Shahzad Wassaf-Shahzad force-pushed the wassaf/5106-add-sentry-webpack-plugin branch from ead5720 to 0f0625f Compare December 14, 2021 11:47
@Wassaf-Shahzad Wassaf-Shahzad merged commit a777c8b into master Dec 14, 2021
@shaidar shaidar deleted the wassaf/5106-add-sentry-webpack-plugin branch March 30, 2022 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add sentry-webpack-plugin
4 participants