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

Creates service on cluster upon "odo push" #4650

Merged
merged 2 commits into from
Apr 22, 2021
Merged

Creates service on cluster upon "odo push" #4650

merged 2 commits into from
Apr 22, 2021

Conversation

dharmit
Copy link
Member

@dharmit dharmit commented Apr 21, 2021

What type of PR is this?
/kind feature

What does this PR do / why we need it:
This PR ensures that an Opreator backed service is created on the cluster only when user does odo push and not when they do odo service create.

Which issue(s) this PR fixes:

Fixes part of the scope mentioned in #4160 (comment)

PR acceptance criteria:

How to test changes / Special notes to the reviewer:
Notes:

  • This PR is targeted only for odo service create; odo push workflow for Operator backed services. Adding labels to the service, deleting service upon doing odo push and Service Catalog related changes will come in subsequent PRs after this one is merged. Doing this to break big piece into smaller chunks.
  • I need to add integration tests to the PR. Integration tests have been modified to use odo push to actually create service on the cluster. Can't think of a new scenario that needs to be added. Open to ideas.

How to test:

  • For Operator backed services, odo service create and odo push. Subsequent odo push would silently pass without any service info (because the service already exists on cluster.)

@openshift-ci-robot openshift-ci-robot added the kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation label Apr 21, 2021
@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dharmit

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. label Apr 21, 2021
@dharmit dharmit requested review from kadel and valaparthvi and removed request for rnapoles-rh April 21, 2021 12:41
@@ -459,39 +465,5 @@ spec:
stdOut = helper.CmdShouldFail("odo", "unlink", "EtcdCluster/example")
Expect(stdOut).To(ContainSubstring("failed to unlink the service"))
})

It("should fail if we delete a link outside of odo (using oc)", func() {
Copy link
Member Author

Choose a reason for hiding this comment

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

Removing this test spec because it's about a scenario where the user does something outside the odo workflow.

@valaparthvi
Copy link
Contributor

Codewise, it looks good to me. I haven't been able to test functionality though, I couldn't get odo catalog list services to work.

@valaparthvi
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Apr 22, 2021
@openshift-merge-robot openshift-merge-robot merged commit cb6e2ca into redhat-developer:master Apr 22, 2021
@dharmit dharmit deleted the fix-4160-labels-and-create-delete-on-push branch April 22, 2021 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation lgtm Indicates that a PR is ready to be merged. Required by Prow.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants