Skip to content
This repository has been archived by the owner on Dec 18, 2019. It is now read-only.

Intly 3698 - add csv for olm & update oauth image #178

Merged
merged 7 commits into from
Nov 7, 2019

Conversation

damienomurchu
Copy link
Contributor

@damienomurchu damienomurchu commented Oct 17, 2019

Motivation

Intly 3698 - add csv for olm & update oauth image for integreatly-operator

What

Add csv for olm & update oauth image

Why

Deploy via olm to rhmi on openshift4

How

Add csv for current version (0.4.1) and update oauth image to quay.io one

Verification Steps

Already verified as part of INTLY-3289

Checklist:

  • Code has been tested locally by PR requester
  • Changes have been successfully verified by another team member

Progress

  • Finished task

Additional Notes

@@ -0,0 +1,4 @@
packageName: mobile-security-service
Copy link
Member

Choose a reason for hiding this comment

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

@damienomurchu I've removed the integreatly- part of this also. I guess it will still be in your one in integr8ly/manifests, but it's out of place here.

enable and disable specific versions of mobile applications on demand
createdAt: "2019-11-04 12:21:00"
support: Red Hat, Inc.
name: mobile-security-service-operator.v0.4.1
Copy link
Member

Choose a reason for hiding this comment

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

@damienomurchu for some reason, when I regenerated with operator-sdk 0.10.1, the version here got a "v" prefix

Copy link
Contributor Author

@damienomurchu damienomurchu Nov 4, 2019

Choose a reason for hiding this comment

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

Yes, same for me, I ended up removing the v prefix for consistency. This value is just metadata however - the image actually used is referenced further down in the csv

Comment on lines +166 to +178
- name: WATCH_NAMESPACE
- name: APP_NAMESPACES
valueFrom:
fieldRef:
fieldPath: metadata.annotations['olm.targetNamespaces']
Copy link
Member

Choose a reason for hiding this comment

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

Changed these from their defaults. This project doesn't use the WATCH_NAMESPACE var for some reason (might change that in future for consistency), and APP_NAMESPACES was set to a default value that we don't really use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@grdryn I don't see a value for metadata.annotations['olm.targetNamespaces'] in the file?

Copy link
Member

Choose a reason for hiding this comment

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

@damienomurchu that annotation is populated by the OLM, based on how it's instructed to install the operator (it's the default value of WATCH_NAMESPACE, when generating a CSV). I haven't actually verified that this works though, I just presume it's a comma-separated list of namespaces.

@grdryn
Copy link
Member

grdryn commented Nov 4, 2019

I've made some changes here. @damienomurchu @laurafitzgerald could you take a quick look?

You can validate the bundle locally by checking out this repo and running:

operator-courier verify deploy/olm-catalog/mobile-security-service-operator/

^there will be one warning about a missing icon, but the return value of the command will be zero (success).

@damienomurchu
Copy link
Contributor Author

damienomurchu commented Nov 4, 2019

@grdryn changes look fine, but will build an image of this and confirm that it installs ok via operator-hub

re: icon, I will fix that, and update the PR

@grdryn
Copy link
Member

grdryn commented Nov 4, 2019

@damienomurchu I don't think you need to build an image, since none of the changes here go into the image -- you should be able to re-use the existing released image, right

Edit: also, we don't have an icon for this service, do we?

@damienomurchu
Copy link
Contributor Author

@grdryn its the built manifest image (via make commands here) that gets picked up by operator hub - I'll build a new manifest image from this PR and publish it to my own quay.io and verify it can be deployed correctly from operatorhub.

I'm just going to do that now and confirm that none of the differences in the csv vs the mss csv in the manifests repo affect the ability to deploy it via olm

@grdryn
Copy link
Member

grdryn commented Nov 4, 2019

@damienomurchu ah right. That doesn't build an actual container image though, does it? I think it just uploads the bundle to a quay.io app repo, right?

@laurafitzgerald
Copy link
Contributor

👀

Copy link
Contributor

@laurafitzgerald laurafitzgerald left a comment

Choose a reason for hiding this comment

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

@grdryn the changes look fine. I verified we wont need the additional crd files as we're using the other ones in the 3.x installer. Ran the command to verify and got the expected results.

@grdryn
Copy link
Member

grdryn commented Nov 5, 2019

I need to make a change here, but it's essentially just that I removed the wrong CRDs, so I need to bring those back & delete the other ones.

Edit: it's not as easy as that unfortunately, since they've got different groups. We're in a very disturbing situation where for some reason the operator-sdk generate openapi command generates the CRDs in the mobilesecurityservice.aerogear.org group, but the controller uses mobile-security-service.aerogear.org (and also that's an already deployed group in RHMI now). I think we need to get the generation command to generate the CRDs for the correct group, but I haven't figured out how to do that yet.

@grdryn
Copy link
Member

grdryn commented Nov 6, 2019

Actually never mind the above comment, I'll do that in a separate PR. The work to do it is growing exponentially, and isn't really related to this PR. This PR has everything correct AFAIK.

@grdryn
Copy link
Member

grdryn commented Nov 7, 2019

@damienomurchu I'm going to merge this now before it gets forgotten. I'm not sure if you're still doing some verification on it or not, but either way if you do happen to notice any issues, do let us know! :)

@grdryn grdryn merged commit 7fa1c87 into aerogear:master Nov 7, 2019
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.

None yet

3 participants