-
Notifications
You must be signed in to change notification settings - Fork 11
Fix Windows environment set issue and change to env to params for compatibility #2
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
Conversation
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.
Thank you very much! ❤️ I didn't noticed this change. I have one feedback though :)
index.js
Outdated
path.join('node_modules', '.bin', 'sentry-cli'), | ||
url ? `--url ${url}` : '', | ||
`--auth-token ${authToken}`, | ||
'releases', |
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 preferred to have releases
in every command. sentryCliExec
is meant to execute sentry cli not specific releases commands. :) Could you please revert that?
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.
With -- parameters you can't do that.
sentry-cli has a certain order rule for that. --org and --project parameters must always come after command.
At first I was going to write like this. this.sentryCliExec('release', new ${releaseName}
);
Two arguments first command, second params. That's why it left named params.
index.js
Outdated
this.log('SENTRY: Release deleted!'); | ||
}, | ||
|
||
sentryCliExec(command) { | ||
sentryCliExec(params) { |
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.
We should get back to command :/ Params would an array or object of options.
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.
If this.sentryCliExec('release', new ${releaseName}
); style ok to you. I'll change the signature with as sentryCliExec(command, params)?
How about now? I think this is better. |
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.
Looks good! Thanks!
Hi.
All environment values changed to -- params for compatibility reasons.
I've tested on my project (on Windows) and working as expected. ( Except --url param which I don't have but I assume it'll work.)
Thank you for this great addon.