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

Use absolute path for discovery doc #457

Merged
merged 3 commits into from
Jul 11, 2018
Merged

Use absolute path for discovery doc #457

merged 3 commits into from
Jul 11, 2018

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

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 Outdated
@@ -189,7 +189,8 @@ def test_execute(self, exec_command, check_output):
organization_name='google-cloud',
package_metadata_yaml='/path/to/pmy.yaml',
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
@@ -128,14 +128,13 @@ class DiscoGapicCodeGenTask(task_base.TaskBase):

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

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

@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

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

@garrettjonesgoogle garrettjonesgoogle garrettjonesgoogle approved these changes

@lukesneeringer lukesneeringer Awaiting requested review from lukesneeringer

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