fix issue https://github.com/kubernetes/kompose/issues/1683 by realgam3 · Pull Request #1684 · kubernetes/kompose

realgam3 · 2023-08-04T17:01:38Z

What type of PR is this?

/kind bug

Which issue(s) this PR fixes:

Fixes #1683

Special notes for your reviewer:

Convert absolute paths to relative paths
Replace Windows paths \ to Linux paths /

pkg/app/app.go

AhmedGrati

Thanks for the PR. left some comments.

pkg/app/app.go

AhmedGrati

Thanks for the PR @realgam3. It looks good overall, but I think adding unit tests showing how the mentioned issues were fixed should be a great addition. wdyt?

realgam3 · 2023-08-24T17:14:48Z

Thanks for the PR @realgam3. It looks good overall, but I think adding unit tests showing how the mentioned issues were fixed should be a great addition. wdyt?

The tests are running on Linux so the windows fix is not relevant to the test. and I already fixed the unit test.
But I can add tests for:

envfile: ./.env ([BUG] pod env_file does not create configmaps #1683 (comment))
Pod with envfile.

What do you think?

AhmedGrati · 2023-08-29T11:06:03Z

@realgam3 yes please you can add/update existing tests for pod envfile.

realgam3 · 2023-08-29T11:22:20Z

@realgam3 yes please you can add/update existing tests for pod envfile.

I will, Thank you!

Edit:
Hi, @AhmedGrati

While I'm trying to create test for pod I am using the command:

./kompose -f script/test/fixtures/configmap-pod/docker-compose.yaml convert --stdout -j

To create a JSON file as other tests and I get the error:

INFO Create kubernetes pod instead of pod controller due to restart policy: no 
FATA cannot convert to one file while specifying a json output file or stdout option

Is it a known issue? or am I missing something?

I've added the test JSON template manually checking that it actually looks like what should have been the output of kompose. if -j and --stdout worked...

AhmedGrati

@realgam3 can we change the test output-k8s-template.json to yaml format?

domehahn · 2023-09-18T10:28:15Z

Hi, it would be really cool if the commit was merged. Thanks.

AhmedGrati · 2023-09-18T11:36:48Z

Hi @domehahn @realgam3, I'm getting time off, I'll review it next week. Thanks for your understanding!

cdrage

Only one comment with regards to using [0] which isn't a good idea.

We also need tests added to kubernetes.go

Sorry for the delayed response in review! It's been a busy couple of weeks.

cdrage · 2023-09-28T15:13:53Z

pkg/transformer/kubernetes/kubernetes.go

 // InitConfigMapForEnv initializes a ConfigMap object
 func (k *Kubernetes) InitConfigMapForEnv(name string, opt kobject.ConvertOptions, envFile string) *api.ConfigMap {
-	envs, err := GetEnvsFromFile(envFile)
+	workDir := filepath.Dir(opt.InputFiles[0])


Using opt.InputFiles[0] throughout the code is kinda scary, I'd advise that we check to see that it's not an empty array.

What would be better is putting this into a function instead similar to

kompose/pkg/transformer/utils.go

Line 331 in ddd5f33

// GetComposeFileDir returns compose file directory

One question?

How you can convert docker-compose files without files?
If opt.InputFiles is empty there is nothing to do... if there is a way to use stdin to get the the docker-compose file that's another issue and we need to prepare for it.

GetComposeFileDir also Doesn't check if the InputFiles is empty and it needs to be fixed because it search for / and it will not find it in windows... it's really bad idea to do path operations yourself.

But if you want me to do more checks let me know...

Just checking to see if opt.InputFiles is empty and then fail is enough.

It's just odd having it check the first element of the array.

It's now ready

AhmedGrati · 2023-10-11T14:41:28Z

ping @realgam3. Any updates on this?

realgam3 · 2023-12-16T14:30:36Z

ping @realgam3. Any updates on this?

Done

realgam3 · 2023-12-18T15:40:02Z

@cdrage, @AhmedGrati anything else you need from me?

AhmedGrati · 2023-12-18T15:42:58Z

@realgam3 gonna review it ASAP.

realgam3 · 2023-12-27T08:02:15Z

@realgam3 gonna review it ASAP.

Any updates / comments?
I don't want to miss the opportunity to push this update, this is an important fix for windows and linux users.

script/test/fixtures/configmap-pod/output-os.yaml

AhmedGrati

Leaving final approve to @cdrage!

k8s-ci-robot · 2024-01-02T19:49:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AhmedGrati, cdrage, realgam3

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~~ [AhmedGrati,cdrage]

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

k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 4, 2023

k8s-ci-robot requested review from hangyan and kadel August 4, 2023 17:01

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 4, 2023

cdrage requested changes Aug 22, 2023

View reviewed changes

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

AhmedGrati reviewed Aug 23, 2023

View reviewed changes

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

realgam3 requested a review from cdrage August 23, 2023 20:09

AhmedGrati reviewed Aug 24, 2023

View reviewed changes

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 Aug 31, 2023

hexolan mentioned this pull request Sep 4, 2023

Unable to read env_file that works with docker-compose up #1287

Closed

AhmedGrati reviewed Sep 4, 2023

View reviewed changes

cdrage requested changes Sep 28, 2023

View reviewed changes

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

fix issue kubernetes#1683

f445590

realgam3 force-pushed the fix-pod-configmaps branch from 5588708 to f445590 Compare December 16, 2023 13:40

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 16, 2023

add tests, die if inputfiles not specified

452b4d3

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 Dec 16, 2023

realgam3 requested a review from cdrage December 16, 2023 14:31

realgam3 added 3 commits December 16, 2023 18:40

fix GetComposeFileDir function and use it

a5b5049

use fixed GetComposeFileDir instead of filepath.Dir

677b91a

return error instead of fatal

56d9d8d

AhmedGrati reviewed Dec 27, 2023

View reviewed changes

script/test/fixtures/configmap-pod/output-os.yaml Show resolved Hide resolved

add pod configmap creation to openshift

71cdba1

AhmedGrati approved these changes Dec 28, 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 Dec 28, 2023

realgam3 mentioned this pull request Dec 31, 2023

[BUG] Conflicting name assigned to configmap generated for compose referencing ".env" files #1793

Closed

cdrage approved these changes Jan 2, 2024

View reviewed changes

cdrage merged commit 92ca12a into kubernetes:main Jan 2, 2024

Porkepix mentioned this pull request Jan 18, 2024

kompose 1.32.0 Homebrew/homebrew-core#160304

Merged

Conversation

realgam3 commented Aug 4, 2023

What type of PR is this?

Which issue(s) this PR fixes:

Special notes for your reviewer:

Uh oh!

Uh oh!

AhmedGrati left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AhmedGrati left a comment

Choose a reason for hiding this comment

Uh oh!

realgam3 commented Aug 24, 2023

Uh oh!

AhmedGrati commented Aug 29, 2023

Uh oh!

realgam3 commented Aug 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AhmedGrati left a comment

Choose a reason for hiding this comment

Uh oh!

domehahn commented Sep 18, 2023

Uh oh!

AhmedGrati commented Sep 18, 2023

Uh oh!

cdrage left a comment

Choose a reason for hiding this comment

Uh oh!

cdrage Sep 28, 2023

Choose a reason for hiding this comment

Uh oh!

realgam3 Sep 28, 2023

Choose a reason for hiding this comment

Uh oh!

cdrage Sep 28, 2023

Choose a reason for hiding this comment

Uh oh!

realgam3 Dec 16, 2023

Choose a reason for hiding this comment

Uh oh!

AhmedGrati commented Oct 11, 2023

Uh oh!

realgam3 commented Dec 16, 2023

Uh oh!

realgam3 commented Dec 18, 2023

Uh oh!

AhmedGrati commented Dec 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

realgam3 commented Dec 27, 2023

Uh oh!

Uh oh!

AhmedGrati left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Jan 2, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

realgam3 commented Aug 29, 2023 •

edited

Loading

AhmedGrati commented Dec 18, 2023 •

edited

Loading