-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
chore(angular-ivy): Add release registry config for @sentry/angular-ivy
#7303
Conversation
2902341
to
fd45178
Compare
.craft.yml
Outdated
@@ -38,6 +38,8 @@ targets: | |||
onlyIfPresent: /^sentry-vue-.*\.tgz$/ | |||
'npm:@sentry/gatsby': | |||
onlyIfPresent: /^sentry-gatsby-.*\.tgz$/ | |||
'npm:@sentry/angular-ivy': | |||
onlyIfPresent: /^sentry-angular-ivy.*\.tgz$/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a - missing at the end?
also any chance the angular one might be matching this one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, thanks f69929c
'npm:@sentry/angular': | ||
onlyIfPresent: /^sentry-angular-.*\.tgz$/ | ||
onlyIfPresent: /^sentry-angular-\d.*\.tgz$/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this would be a bit more explicit:
/^sentry-angular-(?!ivy-).*\.tgz$/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm not sure, this would actually be safer than the \d
regex. Cause theoretically (not saying this happens but nevertheless) sentry-angular-somethingotherthanivy-7.39.0.tgz
would be matched by this regex.
Anyway, I think we're safe with the two current regexes so I'd keep it this way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, yeah, sure, but that also applies to any other package regex we have :D
Maybe it would be better for consistency to then update all regexes to have the \d
part? My main concern is that it may be unclear in the future why this is slightly different then the others, you copy-paste one of them (which one?) etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated all other packages a54a553
This adds automatic release registry updates for the new Ivy compatible Angular SDK
still sort of ref #7265