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

Makefile changes for addition of run bundle in Java Plugin. #84

Merged
merged 27 commits into from Jun 2, 2022
Merged

Makefile changes for addition of run bundle in Java Plugin. #84

merged 27 commits into from Jun 2, 2022

Conversation

laxmikantbpandhare
Copy link
Member

This fix will add changes to Makefile for run bundle. As we need version and group name, I moved Makefile from templates to API.

Able to scaffold out Makefile with required changes and able to generate the bundle files and able to run it too.

Please find below the output after running the bundle command: operator-sdk run bundle docker.io/013859989/memcached-quarkus-operator-bundle:v0.0.98

INFO[0008] Successfully created registry pod: docker-io-013859989-memcached-quarkus-operator-bundle-v0-0-98 
INFO[0008] Created CatalogSource: memcached-quarkus-operator-catalog 
INFO[0008] OperatorGroup "operator-sdk-og" created      
INFO[0008] Created Subscription: memcached-quarkus-operator-v0-1-1-sub 
INFO[0011] Approved InstallPlan install-zx82k for the Subscription: memcached-quarkus-operator-v0-1-1-sub 
INFO[0011] Waiting for ClusterServiceVersion "default/memcached-quarkus-operator.v0.1.1" to reach 'Succeeded' phase 
INFO[0011]   Waiting for ClusterServiceVersion "default/memcached-quarkus-operator.v0.1.1" to appear 
INFO[0025]   Found ClusterServiceVersion "default/memcached-quarkus-operator.v0.1.1" phase: Pending 
INFO[0026]   Found ClusterServiceVersion "default/memcached-quarkus-operator.v0.1.1" phase: Installing 
INFO[0057]   Found ClusterServiceVersion "default/memcached-quarkus-operator.v0.1.1" phase: Succeeded 
INFO[0057] OLM has successfully installed "memcached-quarkus-operator.v0.1.1" 

@laxmikantbpandhare laxmikantbpandhare changed the title Makefile changes for addition of run bundle integration. Makefile changes for addition of run bundle in Java Plugin. May 19, 2022
Copy link
Member

@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.

We shouldn't move the Makefile to create api.

pkg/quarkus/v1alpha/scaffolds/init.go Outdated Show resolved Hide resolved
pkg/quarkus/v1alpha/scaffolds/api.go Outdated Show resolved Hide resolved
@jmrodri
Copy link
Member

jmrodri commented May 21, 2022

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 21, 2022
@laxmikantbpandhare laxmikantbpandhare added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 21, 2022
@laxmikantbpandhare laxmikantbpandhare self-assigned this May 21, 2022
Copy link
Member

@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.

Some more items.

@laxmikantbpandhare
Copy link
Member Author

@jmrodri - Worked on all the review comments. Could you please review once.

@laxmikantbpandhare laxmikantbpandhare removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels May 24, 2022
@laxmikantbpandhare laxmikantbpandhare added api-review Categorizes an issue or PR as actively needing an API review. and removed api-review Categorizes an issue or PR as actively needing an API review. labels May 24, 2022
@jmrodri
Copy link
Member

jmrodri commented Jun 2, 2022

I ran it twice and the update looks great.

$ diff -u Makefile /tmp/Makefile
--- Makefile    2022-06-01 23:21:22.691820093 -0400
+++ /tmp/Makefile       2022-06-01 23:21:14.750800304 -0400
@@ -49,7 +49,7 @@
 .PHONY: bundle
 bundle:  ## Generate bundle manifests and metadata, then validate generated files.
 ## marker
-       cat target/kubernetes/memcacheds.cache.example.com-v1.yml target/kubernetes/jokes.cache.example.com-v1.yml target/kubernetes/kubernetes.yml | operator-sdk generate bundle -q --overwrite --version 0.1.1 --default-channel=stable --channels=stable --package=memcached-quarkus-operator
+       cat target/kubernetes/memcacheds.cache.example.com-v1.yml target/kubernetes/kubernetes.yml | operator-sdk generate bundle -q --overwrite --version 0.1.1 --default-channel=stable --channels=stable --package=memcached-quarkus-operator
        operator-sdk bundle validate ./bundle
 
 .PHONY: bundle-build

pkg/quarkus/v1alpha/api.go Outdated Show resolved Hide resolved
makefileBundleVarFragment = `
##@Bundle
.PHONY: bundle
bundle: ## Generate bundle manifests and metadata, then validate generated files.
Copy link
Member

Choose a reason for hiding this comment

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

I won't hold this PR up but I think we need to make this depend on a build target.

.PHONY: build
build:
    mvn install

.PHONY: bundle
bundle: build
    cat target...

Basically in our other projects if we run make bundle it will do a generation and create the manifests required. But in Java, it won't. But that's because we're not calling anything from make bundle.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes agree, we must add mvn install before generating the bundle else it will fail if the User is not compiled the project before generating the bundle manifest. Let me try to add this to the existing logic.

Copy link
Member Author

Choose a reason for hiding this comment

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

Quick question here, when the User tries the below command at that moment itself it will generate the /target directory.

make docker-build docker-push IMG=quay.io/YOURUSER/memcached-quarkus-operator:0.0.1

Do you still think we still need to add mvn clean install in the bundle structure?

@laxmikantbpandhare
Copy link
Member Author

I ran it twice and the update looks great.

$ diff -u Makefile /tmp/Makefile
--- Makefile    2022-06-01 23:21:22.691820093 -0400
+++ /tmp/Makefile       2022-06-01 23:21:14.750800304 -0400
@@ -49,7 +49,7 @@
 .PHONY: bundle
 bundle:  ## Generate bundle manifests and metadata, then validate generated files.
 ## marker
-       cat target/kubernetes/memcacheds.cache.example.com-v1.yml target/kubernetes/jokes.cache.example.com-v1.yml target/kubernetes/kubernetes.yml | operator-sdk generate bundle -q --overwrite --version 0.1.1 --default-channel=stable --channels=stable --package=memcached-quarkus-operator
+       cat target/kubernetes/memcacheds.cache.example.com-v1.yml target/kubernetes/kubernetes.yml | operator-sdk generate bundle -q --overwrite --version 0.1.1 --default-channel=stable --channels=stable --package=memcached-quarkus-operator
        operator-sdk bundle validate ./bundle
 
 .PHONY: bundle-build

That's great. Thank you for confirming this.

Copy link
Collaborator

@everettraven everettraven left a comment

Choose a reason for hiding this comment

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

Had a couple minor nits but nothing that should hold this PR. Tested the changes locally and everything looks good 👍

pkg/quarkus/v1alpha/api.go Show resolved Hide resolved
pkg/quarkus/v1alpha/api.go Outdated Show resolved Hide resolved
pkg/quarkus/v1alpha/api.go Outdated Show resolved Hide resolved
@laxmikantbpandhare laxmikantbpandhare merged commit 173e51e into operator-framework:main Jun 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants