Skip to content
This repository has been archived by the owner on Jan 19, 2018. It is now read-only.

Add checking if resource is OpenShift specific to properly create OpenShift artifacts #802

Merged
merged 2 commits into from
Nov 22, 2016

Conversation

dymurray
Copy link
Contributor

@dymurray dymurray commented Nov 10, 2016

It appears that this file was copy pasted from the Kubernetes Kubeshift file and wasn't tested. Currently deploying an atomicapp which has a route as an artifact will fail since a route isn't listed as a Kubernetes resource. I tested this deploying an Etherpad atomicapp (https://github.com/fusor/nulecule-library/tree/master/etherpad-atomicapp) on Openshift with an added route artifact with success.

┆Issue is synchronized with this Asana task

@centos-ci
Copy link
Collaborator

Can one of the admins verify this patch?

3 similar comments
@centos-ci
Copy link
Collaborator

Can one of the admins verify this patch?

@centos-ci
Copy link
Collaborator

Can one of the admins verify this patch?

@centos-ci
Copy link
Collaborator

Can one of the admins verify this patch?

@dymurray dymurray changed the title Add checking if resource is OpenShift specific to create properly create OpenShift artifacts Add checking if resource is OpenShift specific to properly create OpenShift artifacts Nov 10, 2016
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) to 65.128% when pulling c4a4d65 on dymurray:ocp into 2b503eb on projectatomic:master.

@cdrage
Copy link
Member

cdrage commented Nov 10, 2016

#dotests

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) to 65.128% when pulling fbe58c7 on dymurray:ocp into 2b503eb on projectatomic:master.

@cdrage
Copy link
Member

cdrage commented Nov 22, 2016

#dotests

@@ -540,5 +540,6 @@ def main():
cli = CLI()
cli.run()


Copy link
Member

Choose a reason for hiding this comment

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

random newline added?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cdrage flake8 was updated last week and since the version is not locked down for CI testing it failed because the latest version requires a double newline between all def statements and the start of the program. Adding this newline passed the CI tests.

Copy link
Member

Choose a reason for hiding this comment

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

@dymurray Ahhh, that makes sense!

@@ -180,6 +180,8 @@ def _generate_kurl(self, obj, namespace, name=None, params=None):
url = self.k8s_api
else:
url = urljoin(self.k8s_apis, "%s/" % api_version)
elif resource in self.oc_api_resources:
url = self.oc_api
Copy link
Member

Choose a reason for hiding this comment

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

This does LGTM, but I'm just going to make sure this still works when creating k8s resources via OpenShift.

oc_object = {"apiVersion": "v1", "kind": "Route", "metadata": {"labels": {"name": "helloapache-route"}, "name": "helloapache-route"}, "spec": {
"host": "$endpoint", "to": [{"kind": "Service", "name": "helloapache-svc"}]}}
a = KubeOpenshiftClient(config)
a.delete(oc_object, "foobar")
Copy link
Member

Choose a reason for hiding this comment

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

👍 for more tests. thank you!

@cdrage
Copy link
Member

cdrage commented Nov 22, 2016

Seems that this works correctly on all fronts! :) I'm going to go ahead and merge this.

@cdrage
Copy link
Member

cdrage commented Nov 22, 2016

Thanks @dymurray

@cdrage cdrage merged commit fba5cbb into projectatomic:master Nov 22, 2016
@centos-ci
Copy link
Collaborator

Can one of the admins verify this patch?

3 similar comments
@centos-ci
Copy link
Collaborator

Can one of the admins verify this patch?

@centos-ci
Copy link
Collaborator

Can one of the admins verify this patch?

@centos-ci
Copy link
Collaborator

Can one of the admins verify this patch?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants