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.

Sign up for GitHub

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

Jump to bottom

Refactor labels #5618

Merged
merged 6 commits into from
Apr 28, 2022
Merged

Refactor labels #5618

merged 6 commits into from
Apr 28, 2022

Conversation

feloy
Copy link
Contributor

@feloy feloy commented Apr 1, 2022 •
edited
Loading

What type of PR is this:

/kind cleanup

What does this PR do / why we need it:

This PR refactors the labels packages to completely hide the keys and instead expose the odo-related concepts: component name, app name, etc.

PR acceptance criteria:

  • Unit test

  • Integration test

  • Documentation

How to test changes / Special notes to the reviewer:

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. label Apr 1, 2022
@netlify
Copy link

netlify bot commented Apr 1, 2022 •
edited
Loading

✅ Deploy Preview for odo-docusaurus-preview canceled.

Name Link
🔨 Latest commit 84bde31
🔍 Latest deploy log https://app.netlify.com/sites/odo-docusaurus-preview/deploys/62679a6d23fc7b00082b41e5

@openshift-ci
Copy link

openshift-ci bot commented Apr 1, 2022

@feloy: The label(s) kind/cleanup cannot be applied, because the repository doesn't have them.

In response to this:

What type of PR is this:

/kind cleanup

What does this PR do / why we need it:

PR acceptance criteria:

  • Unit test

  • Integration test

  • Documentation

How to test changes / Special notes to the reviewer:

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 openshift-ci bot requested review from anandrkskd and cdrage April 1, 2022 15:55
@feloy feloy requested review from rm3l, valaparthvi, kadel and dharmit and removed request for anandrkskd April 1, 2022 15:57
@odo-robot
Copy link

odo-robot bot commented Apr 1, 2022 •
edited
Loading

Unit Tests on commit cee1a4a finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Apr 1, 2022 •
edited
Loading

OpenShift Tests on commit cee1a4a finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Apr 1, 2022 •
edited
Loading

Kubernetes Tests on commit cee1a4a finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Apr 1, 2022 •
edited
Loading

Validate Tests on commit cee1a4a finished successfully.
View logs: TXT HTML

@feloy feloy removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. label Apr 2, 2022
@feloy
Copy link
Contributor Author

feloy commented Apr 4, 2022

@kadel Could you please help review this PR?
I'm still confused about the use of the "app" and "component" labels. Are they really useful?

@feloy feloy force-pushed the refacto-labels branch 2 times, most recently from 4368eee to dba04c8 Compare April 6, 2022 07:12
@feloy feloy changed the title [wip] Refactor labels Refactor labels Apr 7, 2022
@dharmit dharmit removed their request for review April 18, 2022 07:22
@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Apr 22, 2022
valaparthvi
valaparthvi approved these changes Apr 25, 2022
View reviewed changes
pkg/labels/selector_builder.go
func (o selectorBuilder) WithComponent(name string) selectorBuilder {
req, err := labels.NewRequirement(componentLabel, selection.Equals, []string{name})
if err != nil {
panic(err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it okay to panic ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it is. The only error that can happen is when the number of values in the last param's array does not match with the operation in the second argument. Here it will never fail because we need one value with the Eq operation, and we want to fail fast if a developer changes the operation or the last param in a wrong way.

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Apr 25, 2022
@openshift-ci openshift-ci bot removed lgtm Indicates that a PR is ready to be merged. Required by Prow. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. labels Apr 25, 2022
@odo-robot
Copy link

odo-robot bot commented Apr 25, 2022 •
edited
Loading

Windows Tests (OCP) on commit cee1a4a finished successfully.
View logs: TXT HTML

@feloy feloy requested a review from valaparthvi April 25, 2022 08:25
@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Apr 25, 2022
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Apr 26, 2022
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

@valaparthvi
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Apr 26, 2022
@dharmit
Copy link
Member

dharmit commented Apr 27, 2022

@feloy since there is no issue linked/related to this PR, how should a reviewer go about checking what changes to expect as a result of merging this PR? Is this PR changing the labels applied to the Kubernetes resources created by odo, or is it changing the way we create the labels that are eventually applied to the Kubernetes resources created by odo?

@feloy
Copy link
Contributor Author

feloy commented Apr 27, 2022

@feloy since there is no issue linked/related to this PR, how should a reviewer go about checking what changes to expect as a result of merging this PR? Is this PR changing the labels applied to the Kubernetes resources created by odo, or is it changing the way we create the labels that are eventually applied to the Kubernetes resources created by odo?

This is only a code refactoring, the resulting labels in the resources and the selectors used for filtering should be the same before and after this PR

@dharmit
Copy link
Member

dharmit commented Apr 28, 2022

This is only a code refactoring, the resulting labels in the resources and the selectors used for filtering should be the same before and after this PR

👍🏾

/approve

@openshift-ci
Copy link

openshift-ci bot commented Apr 28, 2022

[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 openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. label Apr 28, 2022
@feloy
Copy link
Contributor Author

feloy commented Apr 28, 2022

/retest

@feloy
Copy link
Contributor Author

feloy commented Apr 28, 2022

/override ci/prow/v4.10-integration-e2e

	|  Ginkgo timed out waiting for all parallel nodes to report back!  |
	|                                                                   |
	 -------------------------------------------------------------------
 devfile timed out. path: ./tests/integration/devfile

Tests pass on IBM Cloud

@openshift-ci
Copy link

openshift-ci bot commented Apr 28, 2022

@feloy: Overrode contexts on behalf of feloy: ci/prow/v4.10-integration-e2e

In response to this:

/override ci/prow/v4.10-integration-e2e

  |  Ginkgo timed out waiting for all parallel nodes to report back!  |
  |                                                                   |
   -------------------------------------------------------------------
devfile timed out. path: ./tests/integration/devfile

Tests pass on IBM Cloud

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.

@feloy
Copy link
Contributor Author

feloy commented Apr 28, 2022

/override ci/prow/v4.10-integration-e2e

@openshift-ci
Copy link

openshift-ci bot commented Apr 28, 2022

@feloy: Overrode contexts on behalf of feloy: ci/prow/v4.10-integration-e2e

In response to this:

/override ci/prow/v4.10-integration-e2e

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-merge-robot openshift-merge-robot merged commit feb4960 into redhat-developer:main Apr 28, 2022
cdrage pushed a commit to cdrage/odo that referenced this pull request Aug 31, 2022
@feloy @cdrage
Refactor labels (redhat-developer#5618)
1717725 
* refacto labels

* Getters

* Builder

* Do not export labels

* Move to pkg/labels + doc

* Fix rebase
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cdrage cdrage Awaiting requested review from cdrage

@rm3l rm3l Awaiting requested review from rm3l

@kadel kadel Awaiting requested review from kadel

@valaparthvi valaparthvi Awaiting requested review from valaparthvi

Assignees

@valaparthvi valaparthvi

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. lgtm Indicates that a PR is ready to be merged. Required by Prow.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@feloy @valaparthvi @dharmit @openshift-merge-robot