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

Resolve runtime version from local openshift #581

Merged
merged 2 commits into from Dec 8, 2017

Conversation

djzager
Copy link
Member

@djzager djzager commented Dec 6, 2017

Update the local openshift adapter to read the runtime version off of
the image.

Update the local openshift adapter to read the runtime version off of
the image.
@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Dec 6, 2017
@djzager
Copy link
Member Author

djzager commented Dec 6, 2017

It is worth noting that apb push without using the openshift internal registry does not work at this time:

[2017-12-06T21:14:36.277Z] [DEBUG] handler::apbAddSpec
[2017-12-06T21:14:36.277Z] [DEBUG] Successfully decoded pushed spec:
[2017-12-06T21:14:36.277Z] [DEBUG] version: 1.0
name: hello-world-apb
description: deploys hello-world web application
bindable: False
async: optional
metadata:
  displayName: Hello World Mine (APB)
  longDescription: A sample APB which deploys a containerized Hello World web application
  dependencies: ['docker.io/ansibleplaybookbundle/hello-world:latest']
  providerDisplayName: "Red Hat, Inc."
plans:
  - name: default
    description: A sample APB which deploys Hello World
    free: True
    metadata:
      displayName: Default
      longDescription: This plan deploys a Python web application displaying Hello World
      cost: $0.00
    parameters: []

[2017-12-06T21:14:36.278Z] [DEBUG] Unmarshalled into apb.Spec:
[2017-12-06T21:14:36.278Z] [DEBUG] {ID: Runtime:0 Version:1.0 FQName:hello-world-apb Image: Tags:[] Bindable:false Description:deploys hello-world web application Metadata:map[displayName:Hello World Mine (APB) longDescription:A sample APB which deploys a containerized Hello World web application dependencies:[docker.io/ansibleplaybookbundle/hello-world:latest] providerDisplayName:Red Hat, Inc.] Async:optional Plans:[{ID: Name:default Description:A sample APB which deploys Hello World Metadata:map[displayName:Default longDescription:This plan deploys a Python web application displaying Hello World cost:$0.00] Free:true Bindable:false Parameters:[] BindParameters:[] UpdatesTo:[]}]}

I'm not certain the best way to address this.

@djzager
Copy link
Member Author

djzager commented Dec 6, 2017

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling da01eb0 on djzager:local-openshift into ** on openshift:master**.

@jmrodri jmrodri self-requested a review December 7, 2017 18:01
Copy link
Contributor

@jmrodri jmrodri left a comment

Choose a reason for hiding this comment

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

ACK, though it's weird that the local registry does the runtime parsing in the FetchSpecs but everyone else does it later.

Copy link
Member

@dymurray dymurray left a comment

Choose a reason for hiding this comment

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

This looks good to me. Thanks for checking the local openshift adapter with this work.

@rthallisey
Copy link
Contributor

It is worth noting that apb push without using the openshift internal registry does not work at this time:

Can you explain this further? I think I remember you mentioning it on a call.

@djzager
Copy link
Member Author

djzager commented Dec 8, 2017

Can you explain this further? I think I remember you mentioning it on a call.

@rthallisey when we perform an apb push that doesn't use the internal openshift registry, we simply communicate with the broker 1) the name of our image and 2) the contents of our apb.yml. There is no com.redhat.apb.runtime label to inspect.

We could update the apb push command to also send along the runtime version OR we could come up with a whole new way that we would like for it to work in the kubernetes case with no internal registry. Either way, this PR doesn't fix those issues.

@rthallisey
Copy link
Contributor

We don't have to pass along a runtime since every new apb is going to use the secret for bind creds. Then we would have to default the runtime to 2 in the broker.

@djzager
Copy link
Member Author

djzager commented Dec 8, 2017

That doesn't seem unreasonable. We can't really trust that people won't try and push a v1 runtime image but if we warn on the push that we are assuming the runtime version...then I could be down with that change.

@rthallisey
Copy link
Contributor

If you pull the latest apb-ansible-module you will always get runtime 2. So if you are on a runtime 1 label and you rebuild your container, it should update to the runtime 2 ansible module. A warning on the apb tool side would be good.

We can't really trust that people won't try and push a v1 runtime image

Would you have to install a specific package version of the asb module?

@djzager
Copy link
Member Author

djzager commented Dec 8, 2017

The ansible-asb-modules do not have the label, the apb-base image has the label. The apb tool does not do any inspection of the FROM: line of the Dockerfile before building it and I don't know if the apb tool uses the --no-cache option when building the image (this would make it possible for you to have a cached version of apb-base and use that).

This is a development endpoint, so it is okay if it doesn't work because you messed up, but there are a number of ways that we could think that we are looking at a runtime 2 when it is not. I just don't want that fact to go unnoticed if I'm going to update the broker to handle apb push.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 6b1ad2c on djzager:local-openshift into ** on openshift:master**.

@rthallisey rthallisey merged commit 1cf759f into openshift:master Dec 8, 2017
jianzhangbjz pushed a commit to jianzhangbjz/ansible-service-broker that referenced this pull request May 17, 2018
* Resolve runtime version from local openshift

Update the local openshift adapter to read the runtime version off of
the image.

* Assume apb runtime is v2 on apb push
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-review size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants