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.

Sign up for GitHub

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

Jump to bottom

fix: gracefully handle cases where content URIs are invalid #26

Merged
merged 1 commit into from Aug 17, 2018
Merged

fix: gracefully handle cases where content URIs are invalid #26

merged 1 commit into from Aug 17, 2018

Conversation

0x-r4bbit
Copy link
Contributor

Prior to this commit, all content URIs of application versions are
expected to follow the format <provider>:<identifier>. In case a
content URI doesn't follow this format, an error is thrown.

We now ensure that in such cases, the code still executes but resolves
early with an error or warning respectively.

Fixes #20

0x-r4bbit added a commit to 0x-r4bbit/aragon-cli that referenced this pull request Aug 16, 2018
@0x-r4bbit
uiux(commands/apm): show meaningful error messages
3c827a9 
As discussed in aragon#167, `apm versions` throws when any of the returned
content URIs are invalid. This behaviour will be fixed with aragon/apm.js#26,
so now we're making use of that exposed error or warning message for
a better user experience.

Closes aragon#167
@0x-r4bbit 0x-r4bbit mentioned this pull request Aug 16, 2018
Merged
0x-r4bbit
0x-r4bbit commented Aug 16, 2018
View reviewed changes
src/index.js Outdated
reportType = 'warning'
} else if (!providers[provider]) {
errorMsg = `Provider: ${provider} is not supported`
reportType = 'error'
Copy link
Contributor Author

@0x-r4bbit 0x-r4bbit Aug 16, 2018 •
edited

Choose a reason for hiding this comment

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

As mentioned in the call, I first thought it could be useful to make APM export the different errors types as constants (or preferably enums if we had TypeScript), but I obviously wasn't able to think straight.

For now I went with exposing the report type, so we can make decide on the call side whether we wanna show a warning or an actual error. Let me know if you prefer a different way of exposing this information.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is fine!

@sohkai sohkai requested a review from izqui August 16, 2018 09:31
izqui
izqui approved these changes Aug 17, 2018
View reviewed changes
Copy link
Contributor

@izqui izqui left a comment

Choose a reason for hiding this comment

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

Just a minor comment, looking good!

src/index.js Outdated
reportType = 'warning'
} else if (!providers[provider]) {
errorMsg = `Provider: ${provider} is not supported`
reportType = 'error'
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is fine!

src/index.js Outdated

if (!provider || !location) {
errorMsg = `contentURI: ${contentURI} is invalid.`
reportType = 'warning'
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can remove reportType from here and always report a warning in the CLI.

@izqui
Copy link
Contributor

izqui commented Aug 17, 2018

I think this PR is a good opportunity to fix this as well: #27

@0x-r4bbit 0x-r4bbit mentioned this pull request Aug 17, 2018
Closed
0x-r4bbit added a commit to 0x-r4bbit/aragon-cli that referenced this pull request Aug 17, 2018
@0x-r4bbit
uiux(commands/apm): show meaningful error messages
9e523f4 
As discussed in aragon#167, `apm versions` throws when any of the returned
content URIs are invalid. This behaviour will be fixed with aragon/apm.js#26,
so now we're making use of that exposed error or warning message for
a better user experience.

Closes aragon#167
@0x-r4bbit 0x-r4bbit mentioned this pull request Aug 17, 2018
Merged
@deamme
Copy link
Contributor

deamme commented Aug 17, 2018

Hi! Can you fix these lines also?

apm.js/src/index.js

Lines 88 to 98 in 85056ff

const [provider, location] = contentURI.split(':')
return Object.assign(
manifest,
module,
{ content: { provider, location } }
)
}
)
.catch(() => {
const [provider, location] = contentURI.split(':')

This will fix my issue! Thank you

@0x-r4bbit
fix: gracefully handle cases where content URIs are invalid
d53212f 
Prior to this commit, all content URIs of application versions are
expected to follow the format `<provider>:<identifier>`. In case a
content URI doesn't follow this format, an error is thrown.

We now ensure that in such cases, the code still executes but resolves
early with an error or warning respectively.

It also fixes aragon#27 where parsing content URIs that contain more than
one split character (e.g. `:` in `http://localhost:8080/foo/bar`) splits
the `identifier` as well (which we don't want).

Fixes aragon#20, aragon#27
@0x-r4bbit
Copy link
Contributor Author

@deamme thanks for the heads-up! I've updated it, can you please verify?

@deamme
Copy link
Contributor

deamme commented Aug 17, 2018

@PascalPrecht Works, thanks!

@izqui izqui merged commit 1bd8eaa into aragon:master Aug 17, 2018
@0x-r4bbit 0x-r4bbit deleted the fix/get-application-info branch August 18, 2018 05:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@izqui izqui izqui approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Gracefully handle cases where the URI is not in the format of <provider>:<identifier>
3 participants
@0x-r4bbit @izqui @deamme