fix: env_file configmap name length #1743

ThijsBroersen · 2023-10-20T07:27:29Z

fix filename to configmap name transformer

What type of PR is this?

bug

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

~~I was unable to run tests, could not figure out what system requirements were needed to run the tests.~~
Tests are adjusted to match new configmap name

linux-foundation-easycla · 2023-10-20T07:27:34Z

The committers listed above are authorized under a signed CLA.

✅ login: ThijsBroersen / name: Thijs Broersen (a007ac3, ae0d6c5, 6c21419, 1de8d21, 9cbee96)

k8s-ci-robot · 2023-10-20T07:27:38Z

Welcome @ThijsBroersen!

It looks like this is your first PR to kubernetes/kompose 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kompose has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

cdrage · 2023-10-20T12:07:18Z

pkg/transformer/kubernetes/k8sutils.go

@@ -851,8 +851,15 @@ func GetContentFromFile(file string) (string, error) {
 // FormatEnvName format env name
 func FormatEnvName(name string) string {
 envName := strings.Trim(name, "./")
+ // only take string after the last slash only if the string contains a slash
+ if strings.Contains(envName, "/") {


Thank you! This helps, does this break anything though @AhmedGrati with regards to how we use env?

This also cleans up the really long configmap names, but I'm unsure if they would have conflicts when there are multiple containers.

If you can also add a test case for this under k8sutils_test.go that'd be awesome. Otherwise, good PR 👍

Thanks!
I think it will work as long as env_file names are unique. When different env_file's with the same name are references (from different directories) it will clash.
Could also just only cap at the last, let's say 60 chars? And not filter by last / ?

Do you want anything changed? If not could it be merged soon? I could really use this in a new version of Kompose.

You're right, I think that the names generated should be from env_file names from the last slash.. I really doubt that there will be multiple env file names in different directories for a docker compose project.

HOWEVER if there is, I think that they should be the same name, but one of them filename_1, etc. if there are more of the same file AND Kompose does a warning.

However, I don't feel comfortable merging the PR until we have these two tasks:

tests added to k8sutils_test.go

test for if there are two env file names in different directories

implement renaming to filename_number if there are multiple env files of the same name but in different directories.

Ok, just took a short browse. Added a new env-multiple fixture and test but now sure how to implement this. My thinking would be to check existing configmaps before inserting the new. If a configmap with the same name exists, append a 1 and try again. If that also exists a 2, and so on..
Could you give me a pointer? Or perhaps someone else wants to do this.

Sorry, I didn't actually get it @ThijsBroersen. Can you please clarify it more?

AhmedGrati

Thanks for the PR @ThijsBroersen. Maybe a unit test for this would make it even better 🙏.

AhmedGrati · 2023-10-24T11:39:56Z

.gitpod.yml

+# This configuration file was automatically generated by Gitpod.
+# Please adjust to your needs (see https://www.gitpod.io/docs/introduction/learn-gitpod/gitpod-yaml)
+# and commit this file to your remote git repository to share the goodness with others.
+
+# Learn more from ready-to-use templates: https://www.gitpod.io/docs/introduction/getting-started/quickstart
+
+tasks:
+ - init: go get && go build ./... && go test ./... && make
+ command: go run .
+
+


I think that this should not be added.

AhmedGrati · 2023-10-24T11:45:55Z

pkg/transformer/kubernetes/k8sutils.go

@@ -851,8 +851,15 @@ func GetContentFromFile(file string) (string, error) {
 // FormatEnvName format env name
 func FormatEnvName(name string) string {
 envName := strings.Trim(name, "./")
+ // only take string after the last slash only if the string contains a slash
+ if strings.Contains(envName, "/") {


Sorry, I didn't actually get it @ThijsBroersen. Can you please clarify it more?

cdrage · 2023-10-24T22:48:11Z

I'm good with this PR! LGTM! I understand about writing a golang test, I've opened up an issue here so we make sure we add a future test: #1745

k8s-ci-robot · 2023-10-24T22:48:30Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~OWNERS~~ [cdrage]

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

cdrage · 2023-10-24T22:48:35Z

You'll have to rebase the issue to pass the new tests

AhmedGrati · 2023-10-26T16:21:06Z

ping @ThijsBroersen can u please rebase?

fix filename to configmap name transformer

ThijsBroersen · 2023-10-26T17:29:14Z

I rebased as requested but the new tests will likely still fail because I did not found a way yet to take into account duplicate names. Should I comment out the new test in new_tests.sh ?

AhmedGrati · 2023-10-27T09:48:46Z

Thanks for the contribution @ThijsBroersen. I'm Gonna take care of the tests.

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Oct 20, 2023

k8s-ci-robot requested review from kadel and sebgoa October 20, 2023 07:27

k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Oct 20, 2023

ThijsBroersen mentioned this pull request Oct 20, 2023

[BUG] Generated ConfigMap name too long #1738

Closed

cdrage requested changes Oct 20, 2023

View reviewed changes

ThijsBroersen requested a review from cdrage October 23, 2023 12:43

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 23, 2023

AhmedGrati reviewed Oct 24, 2023

View reviewed changes

ThijsBroersen force-pushed the main branch from c2316a2 to 33bce88 Compare October 24, 2023 12:46

cdrage mentioned this pull request Oct 24, 2023

Add test for PR 1743 #1745

Closed

cdrage approved these changes Oct 24, 2023

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 24, 2023

ThijsBroersen added 4 commits October 26, 2023 17:24

fix: env_file configmap name length

a007ac3

fix filename to configmap name transformer

fix configmap names in tests

ae0d6c5

add env-multiple fixture and test

6c21419

adjust to test

1de8d21

ThijsBroersen force-pushed the main branch from 33bce88 to 1de8d21 Compare October 26, 2023 17:24

disable env-multiple suite

9cbee96

AhmedGrati merged commit 7826534 into kubernetes:main Oct 27, 2023
9 of 10 checks passed

Porkepix mentioned this pull request Jan 18, 2024

kompose 1.32.0 Homebrew/homebrew-core#160304

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: env_file configmap name length #1743

fix: env_file configmap name length #1743

ThijsBroersen commented Oct 20, 2023 •

edited

Loading

linux-foundation-easycla bot commented Oct 20, 2023 •

edited

Loading

k8s-ci-robot commented Oct 20, 2023

cdrage Oct 20, 2023

ThijsBroersen Oct 20, 2023

ThijsBroersen Oct 23, 2023 •

edited

Loading

cdrage Oct 23, 2023

ThijsBroersen Oct 23, 2023

AhmedGrati Oct 24, 2023

AhmedGrati left a comment

AhmedGrati Oct 24, 2023

AhmedGrati Oct 24, 2023

cdrage commented Oct 24, 2023

k8s-ci-robot commented Oct 24, 2023

cdrage commented Oct 24, 2023

AhmedGrati commented Oct 26, 2023

ThijsBroersen commented Oct 26, 2023

AhmedGrati commented Oct 27, 2023

fix: env_file configmap name length #1743

fix: env_file configmap name length #1743

Conversation

ThijsBroersen commented Oct 20, 2023 • edited Loading

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:

linux-foundation-easycla bot commented Oct 20, 2023 • edited Loading

k8s-ci-robot commented Oct 20, 2023

cdrage Oct 20, 2023

Choose a reason for hiding this comment

ThijsBroersen Oct 20, 2023

Choose a reason for hiding this comment

ThijsBroersen Oct 23, 2023 • edited Loading

Choose a reason for hiding this comment

cdrage Oct 23, 2023

Choose a reason for hiding this comment

ThijsBroersen Oct 23, 2023

Choose a reason for hiding this comment

AhmedGrati Oct 24, 2023

Choose a reason for hiding this comment

AhmedGrati left a comment

Choose a reason for hiding this comment

AhmedGrati Oct 24, 2023

Choose a reason for hiding this comment

AhmedGrati Oct 24, 2023

Choose a reason for hiding this comment

cdrage commented Oct 24, 2023

k8s-ci-robot commented Oct 24, 2023

cdrage commented Oct 24, 2023

AhmedGrati commented Oct 26, 2023

ThijsBroersen commented Oct 26, 2023

AhmedGrati commented Oct 27, 2023

ThijsBroersen commented Oct 20, 2023 •

edited

Loading

linux-foundation-easycla bot commented Oct 20, 2023 •

edited

Loading

ThijsBroersen Oct 23, 2023 •

edited

Loading