Feature 1635 - customizing service name with prefix, flag --prefix (-x) by sosan · Pull Request #1833 · kubernetes/kompose

sosan · 2024-02-23T23:48:49Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

The --prefix flag serves the purpose of allowing users to specify prefixes to be added to the names of services

Which issue(s) this PR fixes:

Feature #1635

Special notes for your reviewer:

I haven't had time to document the --prefix flag
documentation will be cooming soon in the following commits.

…a prefix to add the default used in naming services. Added the appendPrefixName function, which takes a pointer to a KomposeObject and an array of prefixes. It appends the first prefix from the array to each service name in the KomposeObject after ensuring consistency using the cleanPrefix function. The cleanPrefix function ensures the format of prefixes, preventing issues with leading or trailing dashes affecting resulting service names. Add unit tests for 'appendPrefixName' and 'cleanPrefix' Signed-off-by: jose luis <2064537+sosan@users.noreply.github.com>

TessaIO

Thanks for the patch @sosan. I left some comments below.

cmd/root.go

pkg/transformer/kubernetes/k8sutils.go

pkg/transformer/kubernetes/k8sutils_test.go

cdrage · 2024-03-01T20:22:26Z

@TessaIO shouldn't we have this as a label instead? or do you think CLI parameter would be fine?

TessaIO · 2024-03-01T20:28:48Z

@TessaIO shouldn't we have this as a label instead? or do you think CLI parameter would be fine?

That's actually true if we want to offer users flexibility to specify per-service prefixes that's the option to go (I mean labels here). wdyt @sosan?

cdrage · 2024-03-01T20:44:55Z

@TessaIO shouldn't we have this as a label instead? or do you think CLI parameter would be fine?

That's actually true if we want to offer users flexibility to specify per-service prefixes that's the option to go (I mean labels here). wdyt @sosan?

Yeah just thinking about it again, I think I'm good for terminal / cli based prefixes being added, we could always customize the name individually via labels instead.

… local variables (prefixArray) from array to string Changed to function name: prependPrefixName Renamed tests to explain better tests behavior Signed-off-by: jose luis <2064537+sosan@users.noreply.github.com>

…to compose yaml

sosan · 2024-03-05T19:18:20Z

Updated cli

Sounds like a good idea to me. That way, we can fine-tune the names individually using labels

TessaIO

Small comments are left below.

pkg/transformer/kubernetes/k8sutils.go

pkg/app/app.go

Signed-off-by: jose luis <2064537+sosan@users.noreply.github.com>

TessaIO

LGTM! Leaving final approve to @cdrage

k8s-ci-robot · 2024-03-06T09:33:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sosan, TessaIO

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [TessaIO]

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

cdrage

One small change requested

cmd/root.go

Signed-off-by: jose luis <2064537+sosan@users.noreply.github.com>

…nged container name :( tests e2e, tests use case from terminal to use prefix complete user_guide.md with prefix -x changed architecture.md to comment new version interface Loader

cdrage

If we are going to add prefix, we should also add suffix...

I do not believe we should have -x either. Prefix and suffix should honestly be fixed after generation. I am not opposed to having this merged in as there is an issue opened for it, but we should also add suffix too.

sosan · 2024-04-02T13:41:33Z

we should also add suffix too.

ok, roger that

TessaIO · 2024-04-18T14:16:08Z

ping @cdrage

cdrage · 2024-04-19T12:20:18Z

cmd/convert.go

 	convertCmd.Flags().BoolVar(&GenerateNetworkPolicies, "generate-network-policies", false, "Specify whether to generate network policies or not")

 	convertCmd.Flags().BoolVar(&WithKomposeAnnotation, "with-kompose-annotation", true, "Add kompose annotations to generated resource")
+	convertCmd.Flags().StringVarP(&GlobalPrefixAdd, "prefix", "x", "", "Adds a custom prefix to service names")


Remove -x please. Are you able to add suffix to this PR as well?

Sure. Add a suffix with flag? Labels?

Just flag like how you did it with suffix

Actually honestly.... I'm not entirely sure why we need this. Thoughts @TessaIO

I think we are adding way too many labels or flags for just suffix : prefix naming.

k8s-ci-robot · 2024-04-19T17:36:54Z

PR needs rebase.

Details

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.

fixed UnaddressableFieldAssign to function addPrefixToServiceName Signed-off-by: jose luis <2064537+sosan@users.noreply.github.com>

Signed-off-by: jose luis <2064537+sosan@users.noreply.github.com>

cdrage · 2024-04-25T16:49:32Z

Hey sorry @sosan for some reason I didn't realize that we reached a conclusion on #1635 (comment) that we should actually close this PR, and I apologize the work you've put into this back and forth.

Going forward I do not think we should be adding too many suffix and prefix / name customizations unless it's "generic", if someone wants to edit the entire -tcp suffix, they could do it with a sed / ctrl +f / ctrl + replace after converting.

There's too much work and labels / parameters to be added to make something small like this happen.

sosan added 2 commits February 23, 2024 21:43

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 23, 2024

k8s-ci-robot requested review from cdrage and kadel February 23, 2024 23:49

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 23, 2024

TessaIO reviewed Feb 29, 2024

View reviewed changes

sosan added 2 commits March 5, 2024 19:29

For simplicity reasons, changed global variables(GlobalPrefixAdd) and…

d2ca94d

… local variables (prefixArray) from array to string Changed to function name: prependPrefixName Renamed tests to explain better tests behavior Signed-off-by: jose luis <2064537+sosan@users.noreply.github.com>

Merge branch 'main' kubernetes/kompose last changes: unify e2e tests …

4e4306b

…to compose yaml

TessaIO reviewed Mar 5, 2024

View reviewed changes

pkg/transformer/kubernetes/k8sutils.go Outdated Show resolved Hide resolved

pkg/app/app.go Outdated Show resolved Hide resolved

removed comments

2d3adca

Signed-off-by: jose luis <2064537+sosan@users.noreply.github.com>

TessaIO approved these changes Mar 6, 2024

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 6, 2024

sosan mentioned this pull request Mar 10, 2024

customize resource name individually via labels kompose.service.name_override #1837

Merged

cdrage requested changes Mar 21, 2024

View reviewed changes

cmd/root.go Outdated Show resolved Hide resolved

sosan added 4 commits March 23, 2024 18:19

changed for a better description

9d8517c

Signed-off-by: jose luis <2064537+sosan@users.noreply.github.com>

userguide and help from terminal

471c5bb

Signed-off-by: jose luis <2064537+sosan@users.noreply.github.com>

Merge branch 'feature-1635-with-labels' into feature-1635

4ad9407

removed incorrect implementation to prefix service resource, only cha…

2f5498e

…nged container name :( tests e2e, tests use case from terminal to use prefix complete user_guide.md with prefix -x changed architecture.md to comment new version interface Loader

cdrage requested changes Apr 2, 2024

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 8, 2024

Merge branch 'main' into feature-1635

eacf397

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 11, 2024

cdrage requested changes Apr 19, 2024

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 19, 2024

sosan added 3 commits April 22, 2024 16:38

Merge branch 'main' into feature-1635

926e421

changed compose types to v2 version

87e2dc7

fixed UnaddressableFieldAssign to function addPrefixToServiceName Signed-off-by: jose luis <2064537+sosan@users.noreply.github.com>

Merge branch 'main' into feature-1635

36b572b

Signed-off-by: jose luis <2064537+sosan@users.noreply.github.com>

cdrage closed this Apr 25, 2024

Conversation

sosan commented Feb 23, 2024

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Uh oh!

TessaIO left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cdrage commented Mar 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TessaIO commented Mar 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cdrage commented Mar 1, 2024

Uh oh!

sosan commented Mar 5, 2024

Uh oh!

TessaIO left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

TessaIO left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Mar 6, 2024

Uh oh!

cdrage left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cdrage left a comment

Choose a reason for hiding this comment

Uh oh!

sosan commented Apr 2, 2024

Uh oh!

TessaIO commented Apr 18, 2024

Uh oh!

cdrage Apr 19, 2024

Choose a reason for hiding this comment

Uh oh!

sosan Apr 25, 2024

Choose a reason for hiding this comment

Uh oh!

cdrage Apr 25, 2024

Choose a reason for hiding this comment

Uh oh!

cdrage Apr 25, 2024

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Apr 19, 2024

Uh oh!

cdrage commented Apr 25, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cdrage commented Mar 1, 2024 •

edited

Loading

TessaIO commented Mar 1, 2024 •

edited

Loading