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

change "from" to "to" for odo link #2334

Merged
merged 1 commit into from Nov 1, 2019
Merged

change "from" to "to" for odo link #2334

merged 1 commit into from Nov 1, 2019

Conversation

danielhelfand
Copy link
Contributor

What kind of PR is this?
/kind cleanup

What does does this PR do / why we need it:

Changes successful odo link message from

Component backend has been successfully linked from the component frontend

to

Component backend has been successfully linked to the component frontend

This reads a lot more clearly since information is being shared from the first component as part of odo link. The current implementation makes it sound as if information is coming from the component specified via the --component flag or via the context being worked in.

Looking for advice on whatever operationName options are available that could potentially not work with this message. Link was the only option considered here.

Which issue(s) this PR fixes:

None

@openshift-ci-robot
Copy link
Collaborator

Hi @danielhelfand. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci-robot openshift-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. Required by Prow. size/XS labels Oct 30, 2019
@amitkrout
Copy link
Contributor

/ok-to-test

@openshift-ci-robot openshift-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. Required by Prow. labels Oct 31, 2019
@kadel
Copy link
Member

kadel commented Oct 31, 2019

Looking for advice on whatever operationName options are available that could potentially not work with this message. Link was the only option considered here.

operationName can have two possible values link or unlink based on the command that is called odo link or odo unlink.

It was probably done like this to to safe some coding, but now this is just complicating the messaging :-(

Component backend has been successfully unlinked to the component frontend

Now this doesn't look right to me. But the original message in odo link was not good either.

We might have to split the messages into two independent messages.
Maybe something like this?

switch operationName {
  case "link":
    log.Successf("%s %s has been successfully linked to the component %s\n", linkType, o.suppliedName, o.Component())
  case "unlink":
    log.Successf("%s %s has been successfully unlinked from the component %s\n", linkType, o.suppliedName, o.Component())
  default:
    return fmt.Errorf("unknwon operation %s", operationName)
}

@kadel
Copy link
Member

kadel commented Oct 31, 2019

/approve

@danielhelfand thank you for helping us making odo much more understandable.

@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 Oct 31, 2019
Copy link
Member

@cdrage cdrage left a comment

Choose a reason for hiding this comment

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

LGTM!

Thanks for the change @danielhelfand

/approve

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cdrage, kadel

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

@cdrage
Copy link
Member

cdrage commented Nov 1, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Nov 1, 2019
@openshift-merge-robot openshift-merge-robot merged commit 21bffa4 into redhat-developer:master Nov 1, 2019
@rm3l rm3l added area/refactoring Issues or PRs related to code refactoring estimated-size/XS (1-5) Rough sizing for Epics. Less than one sprint of work for one person, but smaller than S. labels Jun 16, 2023
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. area/refactoring Issues or PRs related to code refactoring estimated-size/XS (1-5) Rough sizing for Epics. Less than one sprint of work for one person, but smaller than S. lgtm Indicates that a PR is ready to be merged. Required by Prow. ok-to-test Indicates a non-member PR verified by an org member that is safe to test.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants