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

Added quarkus/v1-alpha plugin #4871

Merged
merged 3 commits into from May 10, 2021
Merged

Added quarkus/v1-alpha plugin #4871

merged 3 commits into from May 10, 2021

Conversation

laxmikantbpandhare
Copy link
Member

@laxmikantbpandhare laxmikantbpandhare commented May 4, 2021

Description of the change:
This change will enable the command for Java Plugin.

Motivation for the change:
Implementation of Java Operator's in Operator Framework.

Checklist

If the pull request includes user-facing changes, extra documentation is required:

Copy link
Contributor

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

It is missing:

  • changelog (fragment)
  • sample in the testdata with the java plugin such as it has for go/helm/ansible
    -e2e tests such as it has for go/helm/ansible

Note that is not passing in the sanity because you need to run make generate to re-gen docs.

@jmrodri
Copy link
Member

jmrodri commented May 5, 2021

@camilamacedo86 the samples live in the plugins repo: https://github.com/operator-framework/java-operator-plugins/tree/main/testdata/memcached-quarkus-operator

and it is a v1alpha so it will likely go through massive changes

@jmrodri jmrodri requested review from jmrodri and estroz May 5, 2021 02:15
@estroz
Copy link
Member

estroz commented May 5, 2021

Agreed, samples do not need to live in the operator-sdk repo if the plugin is external. Still needs a changelog, and make sure to specify that this plugin is alpha and subject to breaking changes.

@camilamacedo86
Copy link
Contributor

HI @jmrodri, @estroz

How will we ensure that SDK is generating that properly? So, I think we need a sample in the SDK repo. Also, how we will check its default result and compare the changes and etc? Note that check the samples scaffold is very helpful in the review and development process as we are able to share an example with the users as well.

Also, should we not ensure that the java plugin generated with SDK also pass in the e2e tests such as is deployed, scorecard, bundle tests and etc?

@estroz
Copy link
Member

estroz commented May 5, 2021

How will we ensure that SDK is generating that properly?

Because operator-sdk calls the plugin directly from its repo, which should be creating sample code itself. I do not want one sample per external plugin, as Operator SDK does not necessarily own nor maintain those plugins. That just isn't scalable.

Also, should we not ensure that the java plugin generated with SDK also pass in the e2e tests such as is deployed, scorecard, bundle tests and etc?

We can add integration tests for these, but a checked-in sample is not a requirement for integration tests. These tests can be added in a follow-up since the plugin is very much alpha.

Copy link
Member

@varshaprasad96 varshaprasad96 left a comment

Choose a reason for hiding this comment

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

@laxmikantbpandhare you may have to run make generate to generate the samples again after the change.

@laxmikantbpandhare
Copy link
Member Author

@laxmikantbpandhare you may have to run make generate to generate the samples again after the change.

Yes, I already did and then only did commit. But, sanity is still failing. I will try once again.

@laxmikantbpandhare
Copy link
Member Author

It is missing:

  • changelog (fragment)
  • sample in the testdata with the java plugin such as it has for go/helm/ansible
    -e2e tests such as it has for go/helm/ansible

Note that is not passing in the sanity because you need to run make generate to re-gen docs.

Added changelog (fragment)

@laxmikantbpandhare laxmikantbpandhare changed the title added changes for java plugin [java-plugin] added changes for java plugin May 6, 2021
@estroz
Copy link
Member

estroz commented May 6, 2021

I rebased your commit onto master locally, ran make generate, and saw testdata revert.

Copy link
Member

@estroz estroz left a comment

Choose a reason for hiding this comment

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

/lgtm

go.sum Show resolved Hide resolved
@openshift-ci-robot openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. and removed lgtm Indicates that a PR is ready to be merged. labels May 7, 2021
@openshift-ci-robot
Copy link

New changes are detected. LGTM label has been removed.

laxmikantbpandhare and others added 3 commits May 10, 2021 10:55
Signed-off-by: laxmikantbpandhare <laxmikantpandhare@gmail.com>
made java to quarkus

made v1 to v1alpha

made v1 to v1alpha

Signed-off-by: laxmikantbpandhare <laxmikantpandhare@gmail.com>

made v1 to v1alpha

Signed-off-by: laxmikantbpandhare <laxmikantpandhare@gmail.com>

after make generate command

mod file

kubebuilder version changes

added changelog

Signed-off-by: laxmikantbpandhare <laxmikantpandhare@gmail.com>

modified test-sanity

Signed-off-by: laxmikantbpandhare <laxmikantpandhare@gmail.com>

testdata

Signed-off-by: laxmikantbpandhare <laxmikantpandhare@gmail.com>

did make generate
Signed-off-by: Eric Stroczynski <ericstroczynski@gmail.com>
Copy link
Member

@estroz estroz left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 10, 2021
@estroz estroz changed the title [java-plugin] added changes for java plugin Added quarkus/v1-alpha plugin May 10, 2021
@estroz estroz merged commit 8af50ff into operator-framework:master May 10, 2021
@estroz estroz deleted the java-plugin-changes branch May 10, 2021 19:10
@laxmikantbpandhare laxmikantbpandhare self-assigned this Aug 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants