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

New metering labels #679

Merged
merged 6 commits into from
Nov 29, 2021
Merged

New metering labels #679

merged 6 commits into from
Nov 29, 2021

Conversation

eguzki
Copy link
Member

@eguzki eguzki commented Oct 28, 2021 •
edited
Loading

https://issues.redhat.com/browse/THREESCALE-7750

Spec doc: https://docs.google.com/document/d/1bd9VnZ9jKwDBOOKAJltAtvhz3COUYYmRe3-BGE6fDGE/edit#bookmark=id.27cia5h5kj9j

example of one pod

❯ k get pods apicast-production-1-stm67 -o jsonpath="{.metadata.labels}" | jq '.'
{
  "app": "3scale-api-management",
  "deployment": "apicast-production-1",
  "deploymentConfig": "apicast-production",
  "deploymentconfig": "apicast-production",
  "rht.comp": "3scale",
  "rht.comp_ver": "master",
  "rht.prod_name": "Red_Hat_Integration",
  "rht.prod_ver": "master", 
  "rht.subcomp": "apicast-production",
  "rht.subcomp_t": "application",
  "threescale_component": "apicast",
  "threescale_component_element": "production"
}

@openshift-ci
Copy link

openshift-ci bot commented Oct 28, 2021

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@eguzki eguzki requested a review from miguelsorianod October 28, 2021 16:41
@eguzki eguzki force-pushed the new-metering-labels branch 3 times, most recently from e066ade to 1c6e6c7 Compare November 8, 2021 08:47
@eguzki eguzki force-pushed the new-metering-labels branch from 1c6e6c7 to a579653 Compare November 10, 2021 17:31
@eguzki eguzki marked this pull request as ready for review November 10, 2021 17:39
@eguzki eguzki force-pushed the new-metering-labels branch from a579653 to 9c460cb Compare November 10, 2021 18:12
@eguzki
Copy link
Member Author

eguzki commented Nov 10, 2021

ready for review @miguelsorianod

miguelsorianod
miguelsorianod reviewed Nov 18, 2021
View reviewed changes
pkg/3scale/amp/operator/upgrade.go
return reconcile.Result{Requeue: updated}, nil
}

func (u *UpgradeApiManager) ensurePodTemplateLabels(desired *appsv1.DeploymentConfig) (bool, error) {
Copy link
Contributor

@miguelsorianod miguelsorianod Nov 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

With this changes here we will ensure new labels are added, but the old labels will still be there. Should we clean them too?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added procedure to remove old labels

Copy link
Contributor

Choose a reason for hiding this comment

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

This method will need to be reused in future 3scale versions, without the part that removes the old metering labels. If you want you can add a comment to remember to delete that part

Copy link
Member Author

Choose a reason for hiding this comment

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

done

miguelsorianod
miguelsorianod suggested changes Nov 18, 2021
View reviewed changes
Copy link
Contributor

@miguelsorianod miguelsorianod left a comment

Choose a reason for hiding this comment

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

I left some comments. Overall it looks good 👍

eguzki
eguzki commented Nov 23, 2021
View reviewed changes
pkg/helper/labels.go Outdated Show resolved Hide resolved
@eguzki eguzki force-pushed the new-metering-labels branch from af08577 to 20dfcf8 Compare November 23, 2021 12:54
@eguzki eguzki force-pushed the new-metering-labels branch 2 times, most recently from adb8817 to 7f9d9a1 Compare November 24, 2021 16:20
@eguzki eguzki force-pushed the new-metering-labels branch from 7f9d9a1 to 5a7d2a8 Compare November 24, 2021 16:30
@eguzki
Copy link
Member Author

eguzki commented Nov 26, 2021

/retest

miguelsorianod
miguelsorianod reviewed Nov 29, 2021
View reviewed changes
bundle/manifests/3scale-operator.clusterserviceversion.yaml
control-plane: controller-manager
rht.comp: 3scale
rht.comp_ver: master
Copy link
Contributor

Choose a reason for hiding this comment

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

We will need to be careful when preparing the 2.12 candidate and stable branches about this value too

miguelsorianod
miguelsorianod approved these changes Nov 29, 2021
View reviewed changes
Copy link
Contributor

@miguelsorianod miguelsorianod left a comment

Choose a reason for hiding this comment

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

I left a minor comment. Looks good to me

@codeclimate
Copy link

codeclimate bot commented Nov 29, 2021

Code Climate has analyzed commit 1447c1c and detected 5 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 2
Duplication 2
Style 1

View more on Code Climate.

@eguzki eguzki merged commit 049d287 into master Nov 29, 2021
@eguzki eguzki deleted the new-metering-labels branch November 29, 2021 17:49
@mdujava mdujava mentioned this pull request Feb 4, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miguelsorianod miguelsorianod miguelsorianod approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@eguzki @miguelsorianod