Skip to content

Conversation

@flaper87
Copy link
Contributor

@flaper87 flaper87 commented Nov 1, 2018

OpenStack creds cold be in 3 different paths (etc, home config and
current dir). Instead of re-implementing the logic to find and read the
clouds.yaml file, we should use gophercloud which is the standard
go library for OpenStack.

Note that deployments on OpenStack are currently broken unless there's
a clouds.yaml under /etc/openstack.

Fixes #550

cc @tomassedovic @hardys @russellb @sallyom

@openshift-ci-robot openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Nov 1, 2018
@flaper87
Copy link
Contributor Author

flaper87 commented Nov 1, 2018

/test e2e-openstack

@flaper87 flaper87 force-pushed the openstack-creds-read branch from a93de30 to e7c66f4 Compare November 1, 2018 05:24
@flaper87
Copy link
Contributor Author

flaper87 commented Nov 1, 2018

/test e2e-openstack

@flaper87
Copy link
Contributor Author

flaper87 commented Nov 1, 2018

The failure in the e2e-openstack job is expected: Should be fixed by openshift/release#2048 until a better solution is found.

/me working on it

@hardys
Copy link

hardys commented Nov 1, 2018

+1 was discussing this issue today with @tomassedovic and we mentioned it'd be better to avoid paths completely in the installer and rely on gophercloud instead, thanks for working on it!

@flaper87 flaper87 force-pushed the openstack-creds-read branch 2 times, most recently from 34ff8bb to 2859434 Compare November 2, 2018 08:02
@flaper87
Copy link
Contributor Author

flaper87 commented Nov 2, 2018

/test e2e-openstack

@flaper87
Copy link
Contributor Author

flaper87 commented Nov 2, 2018

FWIW, the e2e-openstack job failure is expected. The patch is doing its job as it's going past the previous failure "invalid argument"

@flaper87
Copy link
Contributor Author

flaper87 commented Nov 2, 2018

/test e2e-aws

1 similar comment
@flaper87
Copy link
Contributor Author

flaper87 commented Nov 2, 2018

/test e2e-aws

@flaper87
Copy link
Contributor Author

flaper87 commented Nov 2, 2018

/test e2e-openstack

@flaper87
Copy link
Contributor Author

flaper87 commented Nov 2, 2018

/test e2e-aws

@flaper87
Copy link
Contributor Author

flaper87 commented Nov 2, 2018

@sallyom @wking This should solve #550 🎉

@crawford
Copy link
Contributor

crawford commented Nov 2, 2018

Can you split this up into multiple commits? Vendoring operations should be in their own commit. Take a look at the repo's history for a sense of how we split things and write the commit messages.

@flaper87 flaper87 force-pushed the openstack-creds-read branch 2 times, most recently from ccd142f to 4da10b4 Compare November 5, 2018 08:34
@flaper87
Copy link
Contributor Author

flaper87 commented Nov 5, 2018

@crawford Thanks for the review! I've split the commits

@tomassedovic
Copy link
Contributor

/assign @tomassedovic

@openshift-ci-robot
Copy link
Contributor

@tomassedovic: GitHub didn't allow me to assign the following users: tomassedovic.

Note that only openshift members and repo collaborators can be assigned.
For more information please see the contributor guide

Details

In response to this:

/assign @tomassedovic

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@tomassedovic
Copy link
Contributor

/assign @tomassedovic

@openshift-ci-robot
Copy link
Contributor

@tomassedovic: GitHub didn't allow me to assign the following users: tomassedovic.

Note that only openshift members and repo collaborators can be assigned.
For more information please see the contributor guide

Details

In response to this:

/assign @tomassedovic

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@tomassedovic
Copy link
Contributor

Strange. I am part of the openshift org. Guess I'm not a collaborator?

@tomassedovic
Copy link
Contributor

/lgtm

@openshift-ci-robot
Copy link
Contributor

@tomassedovic: changing LGTM is restricted to assignees, and only openshift/installer repo collaborators may be assigned issues.

Details

In response to this:

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@wking
Copy link
Member

wking commented Nov 8, 2018

@tomassedovic already reviewed and tried to lgtm this patch.

Maybe he needs to be in a team associated with this repo? From here:

changing LGTM is restricted to assignees, and only openshift/installer repo collaborators may be assigned issues.

GitHub will auto-complete the other openstack-approvers if I try to assign this issue manually, I think it's just something with @tomassedovic's account.

@flaper87 flaper87 force-pushed the openstack-creds-read branch from 36917d5 to be5c247 Compare November 8, 2018 07:08
@flaper87
Copy link
Contributor Author

flaper87 commented Nov 8, 2018

@tomassedovic already reviewed and tried to lgtm this patch.

Maybe he needs to be in a team associated with this repo? From here:

We're all part of this team: https://github.com/orgs/openshift/teams/team-openstack-cloud-provider

I'll verify whether that team is associated with this repo. If so, then it's something related to @tomassedovic account as you said :D

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Nov 8, 2018

@flaper87: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/prow/e2e-openstack 4da10b41bda22448587c749978fd9b1ee29d2d38 link /test e2e-openstack

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@flaper87 flaper87 force-pushed the openstack-creds-read branch from be5c247 to 4f5658a Compare November 8, 2018 07:14
@flaper87
Copy link
Contributor Author

flaper87 commented Nov 8, 2018

According to https://github.com/orgs/openshift/teams/team-openstack-cloud-provider/repositories the team is not associated with this repo. @smarterclayton can you please add this repo to our team? While you're at it, it'd be great to also add release, MCO, and MAO and other repos you think we might need to provide reviews on :)

@flaper87
Copy link
Contributor Author

flaper87 commented Nov 8, 2018

Let's try again

/approve
/lgtm

@openshift-ci-robot
Copy link
Contributor

@flaper87: you cannot LGTM your own PR.

Details

In response to this:

Let's try again

/approve
/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@flaper87
Copy link
Contributor Author

flaper87 commented Nov 8, 2018

@flaper87: you cannot LGTM your own PR.

In response to this:

Let's try again
/approve
/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Smart bot, makes sense! 😄

@flaper87
Copy link
Contributor Author

/assign @hardys

@flaper87
Copy link
Contributor Author

/assign @tomassedovic

@openshift-ci-robot
Copy link
Contributor

@flaper87: GitHub didn't allow me to assign the following users: tomassedovic.

Note that only openshift members and repo collaborators can be assigned.
For more information please see the contributor guide

Details

In response to this:

/assign @tomassedovic

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@tomassedovic
Copy link
Contributor

Let me try this one more time

/lgtm

@openshift-ci-robot
Copy link
Contributor

@tomassedovic: changing LGTM is restricted to assignees, and only openshift/installer repo collaborators may be assigned issues.

Details

In response to this:

Let me try this one more time

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

gophercloud and gophercloud utils are a library and a set of utilities
that provide common functionality to interact with OpenStack clouds,
configurations, etc.

We need these libraries to manage OpenStack configs as it's done
upstream and for future work like adding an OpenStack destroyer.
OpenStack creds cold be in 3 different paths (etc, home config and
current dir). Instead of re-implementing the logic to find and read the
clouds.yaml file, we should use gophercloud which is the standard
go library for OpenStack.

Note that deployments on OpenStack are currently broken unless there's
a clouds.yaml under /etc/openstack.

Fixes openshift#550
@flaper87 flaper87 force-pushed the openstack-creds-read branch from 4f5658a to 40e438c Compare November 13, 2018 10:37
@hardys
Copy link

hardys commented Nov 13, 2018

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 13, 2018
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flaper87, hardys, tomassedovic

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit 3c5b2be into openshift:master Nov 13, 2018
@openshift-ci-robot
Copy link
Contributor

@petr-muller: GitHub didn't allow me to assign the following users: tomassedovic.

Note that only openshift members and repo collaborators can be assigned.
For more information please see the contributor guide

Details

In response to this:

/assign @tomassedovic

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@tomassedovic
Copy link
Contributor

/assign @tomassedovic

@wking
Copy link
Member

wking commented Nov 14, 2018

Hooray for both the merge and @tomassedovic becoming assignable ;). For future reference, @tomassedovic do you know what changed for your to become assignable?

@tomassedovic
Copy link
Contributor

@wking

@eparis fixed it for me. He said: "/assign requires you to have read access to the repo, not just the repo be public...."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants