Skip to content
This repository was archived by the owner on Mar 1, 2022. It is now read-only.

Use absolute path for discovery doc#457

Merged
andreamlin merged 3 commits intogoogleapis:masterfrom
andreamlin:discovery_doc
Jul 11, 2018
Merged

Use absolute path for discovery doc#457
andreamlin merged 3 commits intogoogleapis:masterfrom
andreamlin:discovery_doc

Conversation

@andreamlin
Copy link
Contributor

@andreamlin andreamlin commented Jul 11, 2018 •
edited
Loading

This allows the artman command line call to run from any directory, e.g. google-cloud-java, not just from inside the discovery_artifact_manager directory.

@andreamlin
Copy link
Contributor Author

andreamlin commented Jul 11, 2018

PTAL

@andreamlin andreamlin mentioned this pull request Jul 11, 2018
Merged
garrettjonesgoogle
garrettjonesgoogle reviewed Jul 11, 2018
View reviewed changes
test/tasks/test_gapic.py
discovery_doc="/path/to/compute.v1.json",
toolkit_path='/path/to/toolkit'
toolkit_path='/path/to/toolkit',
root_dir='root_dir'

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

alexander-fenster
alexander-fenster reviewed Jul 11, 2018
View reviewed changes
artman/tasks/gapic_tasks.py
def execute(self, language, toolkit_path, discovery_doc,
gapic_yaml, package_metadata_yaml,
gapic_code_dir, api_name, api_version, organization_name):
gapic_code_dir, api_name, api_version, organization_name, root_dir):

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@alexander-fenster
Copy link
Contributor

alexander-fenster commented Jul 11, 2018

I might be missing something - it's unclear for me how the root dir will be used. Why it only applies to one parameter (discovery doc)?

@andreamlin
Copy link
Contributor Author

andreamlin commented Jul 11, 2018

@alexander-fenster
(Copied from a previous comment)
The problem is that artman resolves all filepaths in the config to be relative to the config filepath and assumes that the artman config file is at the highest (or equal) level directory compared to the files in the config.
For example, in googleapis, for google/pubsub/artman_pubsub.yaml, all the pubsub API files are located at or below that directory level, like google/pubsub/v1/pubsub.proto.

However, in discovery-artifact-manager, the relevant discovery document is not in the same directory path as the artman config.
For example, for gapic/google/compute/artman_compute.yaml, the relevant discovery doc is at discoveries/compute.v1.json.

PTAL

garrettjonesgoogle
garrettjonesgoogle approved these changes Jul 11, 2018
View reviewed changes
Copy link
Member

@garrettjonesgoogle garrettjonesgoogle left a comment

Choose a reason for hiding this comment

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

Ok, let's go with this. LGTM

@andreamlin andreamlin merged commit c5ce22d into googleapis:master Jul 11, 2018
@andreamlin andreamlin deleted the discovery_doc branch July 11, 2018 21:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@lukesneeringer lukesneeringer Awaiting requested review from lukesneeringer

2 more reviewers

@alexander-fenster alexander-fenster alexander-fenster left review comments

@garrettjonesgoogle garrettjonesgoogle garrettjonesgoogle approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@andreamlin @alexander-fenster @garrettjonesgoogle