Skip to content

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

Merged
merged 3 commits into from
May 27, 2020

Conversation

sinankeskin
Copy link
Contributor

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.

Copy link
Member

@Exelord Exelord left a 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',
Copy link
Member

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?

Copy link
Contributor Author

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) {
Copy link
Member

@Exelord Exelord May 27, 2020

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.

Copy link
Contributor Author

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)?

@sinankeskin
Copy link
Contributor Author

How about now? I think this is better.

Copy link
Member

@Exelord Exelord 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! Thanks!

@Exelord Exelord merged commit ea7d155 into EmberExperts:master May 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants