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

add tags/groups support for gcp #51

Closed
wants to merge 1 commit into from

Conversation

stephansnyt
Copy link

No description provided.

@adammck
Copy link
Owner

adammck commented Nov 5, 2016

Hello, @stephansnyt! This looks good to me, but can we get a test in parser_test.go? Should just be a matter of adding an example to the giant fixture and the expected output.

@mblarsen
Copy link
Contributor

@stephansnyt could you add digitalocean_droplet to the case? Then it will work on digital ocean tags as well.

case "digitalocean_droplet", "google_compute_instance":

To the test you can add these tags to the existing digitalocean_droplet:

"tags.#": "2",
"tags.1": "staging",
"tags.2": "webserver"

.. and this at the and of the expected output:

    "webserver": ["192.168.0.3"],
    "staging": ["192.168.0.3"]

(Remember to put a comma efter the previous value.)

I tested your change with digitalocean_droplet change on top of it as well as with the above test fixtures and it all went well :)

Thanks

@mblarsen
Copy link
Contributor

@stephansnyt @adammck Any change this could be merged? I'll gladly make a PR that includes both providers + tests.

@yanc0
Copy link

yanc0 commented Jan 17, 2017

I have created a PR (stephansnyt#1) on the @stephansnyt 's master branch. Hope He'll accept it and then we can merge this PR. For DO, maybe we can create its own PR for tracking purposes? What do you think @mblarsen?

Thanks

@mblarsen
Copy link
Contributor

That's great @yanc0 — I can add the DO parts as soon as this is merged. But both @stephansnyt and @adammck seems to be really busy, crossing fingers for near-future merge :)

@adammck
Copy link
Owner

adammck commented Jan 19, 2017

Merged the feature and tests in #55. Thanks!

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.

4 participants