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

BUG: Fix access_urls for some services #152

Merged
merged 3 commits into from
Sep 11, 2017

Conversation

dopplershift
Copy link
Member

Catalogs are allowed to specify the name of a service that is contained
within a compound service. Siphon was not properly creating the access
urls dictionary for this case.

Catalogs are allowed to specify the name of a service that is contained
within a compound service. Siphon was not properly creating the access
urls dictionary for this case.
Copy link
Collaborator

@lesserwhirls lesserwhirls 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.

Copy link
Contributor

@jrleeman jrleeman left a comment

Choose a reason for hiding this comment

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

One question that is more for me to understand the compound service.

service_name = metadata['serviceName']
all_service_dict = {}
for service in all_services:
all_service_dict[service.name] = service
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be in an else? (i.e. should the compound service be in the dictionary or just it's sub services?)

Copy link
Member Author

Choose a reason for hiding this comment

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

You want both. I initially had it as an else, and it (rightly) failed tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Seems that we weren't using the right variable to point to the 64-bit
conda, so we were running 32-bit conda on 64-bit platforms. Oops.
We've asked CodeCov to track this, so we should probably no longer
ignore coverage results for the test files.
@jrleeman jrleeman merged commit e5731ad into Unidata:master Sep 11, 2017
@dopplershift dopplershift deleted the fix-cat-services branch September 11, 2017 21:03
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.

3 participants