-
Notifications
You must be signed in to change notification settings - Fork 95
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
fix: Allow to use the COMPOSE_FILE variable in finch compose #994
Conversation
A test of the |
…e finch compose command The following pull request attempts to modify the finch compose command to allow the COMPOSE_FILE environment variable. - runfinch/finch#994 Therefore, this fix adds an e2e test to run finch compose command with the COMPOSE_FILE environment variable. Signed-off-by: Hayato Kiwata <haytok@amazon.co.jp>
…nch compose command The following pull request attempts to modify finch compose command to allow the COMPOSE_FILE environment variable. - runfinch/finch#994 Therefore, this fix adds an e2e test to run finch compose command with the COMPOSE_FILE environment variable. Signed-off-by: Hayato Kiwata <haytok@amazon.co.jp>
Added an e2e test for |
CI in The error messages are as follows. Details
CI is failing due to the following logic not completing successfully, possibly Flaky Test. This may be related to the draft that austinvazquez is creating. However, the root cause of this CI failure is not clear at this time. Ask/QuestionI would appreciate any guidance from other maintainers regarding appropriate approaches to addressing this matter. |
#172) …nch compose command The following pull request attempts to modify finch compose command to allow the COMPOSE_FILE environment variable. - runfinch/finch#994 Therefore, this fix adds an e2e test to run finch compose command with the COMPOSE_FILE environment variable. Issue #, if available: N/A *Description of changes:* The details are described in this commit message. *Testing done:* Yes - [x] I've reviewed the guidance in CONTRIBUTING.md #### License Acceptance By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. --------- Signed-off-by: Hayato Kiwata <haytok@amazon.co.jp>
@haytok Can you bump common-tests to |
Sure, i'll update go.mod and go.sum. |
Thanks for checking!!!, @pendo324 I bumped common-tests to v0.7.23. |
Thanks, the change looks good to merge after the new test passes |
Looks like the new test failed on Windows |
I'll check the root cause of the failure. |
I will demonstrate the results of the operational verification conducted in the Windows environment. Details
Set COMPOSE_FILE env var ~
$env:COMPOSE_FILE = "a.yaml" Check env var ~
Get-ChildItem env:
Name Value
---- -----
...
COMPOSE_FILE a.yaml
... Execute finch compose up ~
.\_output\bin\finch.exe compose up
INFO[0000] Creating network finch_default
INFO[0000] Ensuring image busybox
...
INFO[0000] Creating container finch-echo_env-1
INFO[0000] Attaching to logs
echo_env-1 |PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
echo_env-1 |DEFAULT=1
echo_env-1 |HOME=/root
INFO[0000] Container "finch-echo_env-1" exited
INFO[0000] All the containers have exited
INFO[0000] Stopping containers (forcibly)
INFO[0000] Stopping container finch-echo_env-1 It was possible to execute the "finch compose up" command using the Docker Compose file ( Therefore, I will proceed to investigate the tests in the Windows environment. |
There seemed to be an error because the process to convert the file path specified by the environment variable in the e2e test to a path in WSL was not performed. I'll think about how to resolve it. |
I found out why the test is failing. Details
> git diff .\cmd\finch\nerdctl.go
diff --git a/cmd/finch/nerdctl.go b/cmd/finch/nerdctl.go
index f363171..820ef70 100644
--- a/cmd/finch/nerdctl.go
+++ b/cmd/finch/nerdctl.go
@@ -234,13 +234,17 @@ func (nc *nerdctlCommand) run(cmdName string, args []string) error {
passedEnvs := []string{
"COSIGN_PASSWORD", "AWS_ACCESS_KEY_ID", "AWS_SECRET_ACCESS_KEY",
- "AWS_SESSION_TOKEN",
+ "AWS_SESSION_TOKEN", "COMPOSE_FILE",
}
var passedEnvArgs []string
for _, e := range passedEnvs {
v, b := nc.systemDeps.LookupEnv(e)
if b {
+ if e == "COMPOSE_FILE" {
+ path, _ := convertToWSLPath(nc.systemDeps, v)
+ v = path
+ }
passedEnvArgs = append(passedEnvArgs, fmt.Sprintf("%s=%s", e, v)) Test locally on windows and darwin and then create the commit again. |
Oh yeah, makes sense. Nice find! |
@austinvazquez Thank you for fixing the CI and shareing the Info!!! I will rebase and make an additional commit! |
nerdctl allows us to launch containers using a Docker Compose file specified in the COMPOSE_FILE environment variable. For example, suppose we have created the following Docker Compose file. ``` > cat a.yaml services: test: image: amazonlinux:2023 ``` By specifying this Docker Compose file in the COMPOSE_FILE environment variable and running nerdctl compose up, we can start the container. ``` > sudo COMPOSE_FILE=a.yaml _output/nerdctl compose up INFO[0000] Ensuring image amazonlinux:2023 INFO[0000] Re-creating container nerdctl-test-1 INFO[0000] Attaching to logs INFO[0000] Container "nerdctl-test-1" exited INFO[0000] All the containers have exited INFO[0000] Stopping containers (forcibly) INFO[0000] Stopping container nerdctl-test-1 ``` However, since the COMPOSE_FILE environment variable is not passed in finch compose, the following error occurs. ``` > COMPOSE_FILE=a.yaml finch compose up FATA[0000] no configuration file provided: not found FATA[0000] exit status 1 ``` And this bug is reported in the following issue. - runfinch#347 Therefore, this fix allows the finch compose command to use the Docker Compose file specified in the COMPOSE_FILE environment variable. Signed-off-by: Hayato Kiwata <haytok@amazon.co.jp>
This commit bumps common-tests to v0.7.23. Signed-off-by: Hayato Kiwata <haytok@amazon.co.jp>
Add a process to convert the path of the file specified in the COMPOSE_FILE variable to WSL path in Windows environment. Signed-off-by: Hayato Kiwata <haytok@amazon.co.jp>
e998e10
to
207ba50
Compare
Hi, @pendo324 Fix the code, rebase and push. |
LGTM, thanks! |
Thanks for approving!!! I have hecked PR again and found no problem, so merge. |
🤖 I have created a release *beep* *boop* --- ## [1.2.1](v1.2.0...v1.2.1) (2024-07-02) ### Build System or External Dependencies * **deps:** Bump github.com/aws/aws-sdk-go-v2 from 1.27.0 to 1.27.1 ([#963](#963)) ([4c2dc12](4c2dc12)) * **deps:** Bump github.com/aws/aws-sdk-go-v2 from 1.27.1 to 1.27.2 ([#974](#974)) ([54aa67c](54aa67c)) * **deps:** bump github.com/aws/aws-sdk-go-v2 from 1.27.2 to 1.30.0 ([#991](#991)) ([bbcb8e7](bbcb8e7)) * **deps:** Bump github.com/docker/cli from 26.1.3+incompatible to 26.1.4+incompatible ([#973](#973)) ([f774e2d](f774e2d)) * **deps:** bump github.com/docker/cli from 26.1.4+incompatible to 27.0.2+incompatible ([#999](#999)) ([0244698](0244698)) * **deps:** bump github.com/docker/cli from 27.0.2+incompatible to 27.0.3+incompatible ([#1005](#1005)) ([c801e69](c801e69)) * **deps:** Bump github.com/docker/docker from 26.1.3+incompatible to 26.1.4+incompatible ([#972](#972)) ([05b9c05](05b9c05)) * **deps:** bump github.com/docker/docker from 26.1.4+incompatible to 27.0.1+incompatible ([#996](#996)) ([1f68260](1f68260)) * **deps:** bump github.com/docker/docker from 27.0.1+incompatible to 27.0.2+incompatible ([#1001](#1001)) ([50a639b](50a639b)) * **deps:** bump github.com/docker/docker from 27.0.2+incompatible to 27.0.3+incompatible ([#1006](#1006)) ([537abad](537abad)) * **deps:** bump github.com/spf13/cobra from 1.8.0 to 1.8.1 ([#983](#983)) ([7b2bed6](7b2bed6)) * **deps:** bump golang.org/x/image from 0.12.0 to 0.18.0 ([#998](#998)) ([398658e](398658e)) * **deps:** Bump golang.org/x/text from 0.15.0 to 0.16.0 ([#964](#964)) ([8a3973a](8a3973a)) * **deps:** Bump golang.org/x/tools from 0.21.0 to 0.22.0 ([#967](#967)) ([3921b00](3921b00)) * **deps:** bump k8s.io/apimachinery from 0.30.1 to 0.30.2 ([#981](#981)) ([c8ebf20](c8ebf20)) * **deps:** Bump submodules and dependencies ([#1008](#1008)) ([6134a5a](6134a5a)) * **deps:** Bump submodules and dependencies ([#949](#949)) ([b5ee424](b5ee424)) ### Bug Fixes * add SOCI snapshotter hash check ([#985](#985)) ([563f346](563f346)) * Allow to use the COMPOSE_FILE variable in finch compose ([#994](#994)) ([17d4bc8](17d4bc8)) * Enable `finch support-bundle generate` to execute on Windows whe… ([#976](#976)) ([9c1caf0](9c1caf0)) * update snapshotters reference ([#986](#986)) ([06b9027](06b9027)) * verify shasum for finch dependencies ([#969](#969)) ([9d85f25](9d85f25)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
nerdctl allows us to launch containers using a Docker Compose file specified in the COMPOSE_FILE environment variable.
For example, suppose we have created the following Docker Compose file.
By specifying this Docker Compose file in the COMPOSE_FILE environment variable and running nerdctl compose up, we can start the container.
However, since the COMPOSE_FILE environment variable is not passed in finch compose, the following error occurs.
And this bug is reported in the following issue.
Therefore, this fix allows the finch compose command to use the Docker Compose file specified in the COMPOSE_FILE environment variable.
Issue #, if available: #347
Description of changes: The details are described in this commit message.
Testing done: Yes
License Acceptance
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.