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

Allow specifying additional labels to Piped's service manifest #2125

Merged

Conversation

gotyoooo
Copy link
Contributor

What this PR does / why we need it:
Change to be able to add optional labels to the pipe service.
Labels are useful because they may be used for purposes other than those used by Piped.
If necessary, add it to pipecd, site, but how about it?

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:

NONE

@pipecd-bot
Copy link
Collaborator

COVERAGE

Code coverage for golang is 32.53%. This pull request does not change code coverage.

@nghialv
Copy link
Member

nghialv commented Jun 24, 2021

@gotyoooo Thank you for your pull request.
I think that is a nice idea. I have just left a comment.

@@ -16,6 +16,9 @@ service:
type: ClusterIP
port: 9085

# Optional additional labels to add to the Service
# serviceLabels: {}

Copy link
Member

Choose a reason for hiding this comment

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

I know in some cases, we don't want to add labels to all manifests, but just for service manifest.
How about moving this field to be an inner field of service?

service:
    enabled: true
    type: ClusterIP
    port: 9085
    additionalLabels: {}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Certainly it's better!
I will change it.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you.

@gotyoooo gotyoooo force-pushed the Add-Values-serviceLabels-to-chart branch from 03ff019 to 013e6c3 Compare June 24, 2021 07:34
@gotyoooo gotyoooo force-pushed the Add-Values-serviceLabels-to-chart branch from 013e6c3 to 3aa1056 Compare June 24, 2021 07:39
@nghialv nghialv changed the title Add Values.serviceLabels to piped chart Allow specifying additional labels to Piped's service manifest Jun 24, 2021
@nghialv
Copy link
Member

nghialv commented Jun 24, 2021

Thank you.
/lgtm

@khanhtc1202
Copy link
Member

Nice, thx
/approve

@pipecd-bot
Copy link
Collaborator

APPROVE

This pull request is APPROVED by khanhtc1202.

Approvers can cancel the approval by writing /approve cancel in a comment. Any additional commits also will change this pull request to be not-approved.

@pipecd-bot
Copy link
Collaborator

COVERAGE

Code coverage for golang is 32.47%. This pull request does not change code coverage.

@pipecd-bot pipecd-bot merged commit a97a99c into pipe-cd:master Jun 24, 2021
@gotyoooo gotyoooo deleted the Add-Values-serviceLabels-to-chart branch June 24, 2021 07:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants