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

feat(Allow usage of apply command for App CR) : Rebind without the need to delete and create again #103

Merged
merged 6 commits into from
May 29, 2019

Conversation

camilamacedo86
Copy link
Contributor

@camilamacedo86 camilamacedo86 commented May 24, 2019

Motivation

https://issues.jboss.org/browse/AEROGEAR-9268

What

  • Ensure that always will have just 1 ConfigMap created with the latest name of APP CR
  • Allow usage of the apply command
  • Fix debug after all changes.
  • Fix/improve tests after rebase
  • Use the model from MSS instead of duplicate it.
  • Upgrade operator-sdk from 0.7.0 to the latest version available 0.8.1

Verification Steps

Use the image: docker.io/cmacedo/mobile-security-service-operator:AEROGEAR-9268

  1. Install the oper
  2. Create the app in the specific namespace
  3. Check that a configMap was created for this app
  4. Update the name into the Example APP CR
  5. Run the command make create-app again
  6. It should have just one confgMap with the new name and not 2.

Checklist:

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

Progress

  • Finished task
  • TODO

Additional Notes

Screenshot 2019-05-28 at 10 45 44

@camilamacedo86 camilamacedo86 changed the title feat(ConfigMap) : Rebind without the need to delete and create again … feat(ConfigMap) : Rebind without the need to delete and create again (use apply) May 24, 2019
@camilamacedo86 camilamacedo86 added the work in progress work in progress label May 24, 2019
@camilamacedo86 camilamacedo86 force-pushed the AEROGEAR-9268 branch 5 times, most recently from 9c54892 to 12fcfa7 Compare May 28, 2019 09:47
@camilamacedo86 camilamacedo86 removed the work in progress work in progress label May 28, 2019
@camilamacedo86 camilamacedo86 force-pushed the AEROGEAR-9268 branch 3 times, most recently from d126672 to 40bd36d Compare May 28, 2019 11:43
@camilamacedo86 camilamacedo86 force-pushed the AEROGEAR-9268 branch 3 times, most recently from a00c4e5 to dca48fc Compare May 28, 2019 13:23
@@ -6,6 +6,7 @@ vendor/
build/_output
build/_test
cmd/manager/debug
cmd/manager/main
Copy link
Contributor

Choose a reason for hiding this comment

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

combine both with cmd/manager/* ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @damienomurchu,

Tks for your time. Unfortunately, it is not possible. The main.go is in this dir.

Copy link
Contributor

@damienomurchu damienomurchu left a comment

Choose a reason for hiding this comment

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

Changes look good, needs a rebase which you know.

Haven't verified changes, or run tests yet

Copy link
Member

@davidffrench davidffrench left a comment

Choose a reason for hiding this comment

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

Good work @camilamacedo86 . Just reviewed the code, will verify once items are addressed.

Makefile Outdated Show resolved Hide resolved
pkg/controller/mobilesecurityservice/controller.go Outdated Show resolved Hide resolved
pkg/controller/mobilesecurityserviceapp/controller.go Outdated Show resolved Hide resolved
// If the Service instance was not found and/or is marked to be deleted
// OR
// if the APP CR was marked to be deleted
if (isAppMarkedToBeDeleted && len(instance.GetFinalizers()) > 0) || mssInstance == nil || mssInstance.GetDeletionTimestamp() != nil {
Copy link
Member

Choose a reason for hiding this comment

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

@camilamacedo86 Since the mssInstance is initalised as &mobilesecurityservicev1alpha1.MobileSecurityService{} https://github.com/aerogear/mobile-security-service-operator/pull/103/files#diff-f0c3b61d21a85c4c894842a29ca22af2R151 . Is there a possibility that the mssInstance will ever be nil? And if there is, in what scenario will it be nil?

The way I read this logic is that if the mssInstance is nil and the app CR is not marked for deletion, it will still be deleted. This could be very dangerous if the mssInstance is returned as nil in a case where we do not want to delete the app CR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The MSS will just be nill when the Service CR/CRD was deleted from the cluster, so if the mss instance is nil each means that we should allow deleting the APP CR.

Copy link
Member

Choose a reason for hiding this comment

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

This is the only scenario where it can be nil?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I cannot see any other scenario for the MSS be nil. Are you able to see any other scenario?

pkg/controller/mobilesecurityservicedb/controller.go Outdated Show resolved Hide resolved
@camilamacedo86 camilamacedo86 changed the title feat(ConfigMap) : Rebind without the need to delete and create again (use apply) feat(Allow usage of apply command for App CR) : Rebind without the need to delete and create again May 28, 2019
@camilamacedo86 camilamacedo86 force-pushed the AEROGEAR-9268 branch 3 times, most recently from 7807034 to fcec1d5 Compare May 28, 2019 22:38
@laurafitzgerald
Copy link
Contributor

@camilamacedo86 I've completed the verification steps and the changes are working as expected based on the second last commit. I see there are some feedback items and the build is failing on a new commit now so I haven't added an approve yet. I added a verified label. Feel free to ping me when you want a recheck.

@camilamacedo86 camilamacedo86 force-pushed the AEROGEAR-9268 branch 2 times, most recently from 846f237 to 72c3054 Compare May 29, 2019 10:46
@camilamacedo86 camilamacedo86 force-pushed the AEROGEAR-9268 branch 3 times, most recently from 7f3e541 to dbe921c Compare May 29, 2019 11:26
Copy link
Contributor

@damienomurchu damienomurchu left a comment

Choose a reason for hiding this comment

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

Reviewed code & re-verified functionality. Approving as seems you clarified all PR comments and don't see a reason not to approve now

@camilamacedo86 camilamacedo86 merged commit 7fdfb88 into aerogear:master May 29, 2019
@camilamacedo86 camilamacedo86 deleted the AEROGEAR-9268 branch May 29, 2019 11:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants