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

Change whole image url, dont care about last value #241

Conversation

jindrichskupa
Copy link

@jindrichskupa jindrichskupa commented Sep 30, 2021

Fixes

Changes

  1. Added --dry run param only to show new task definition
  2. Fixed some linter issues: SC2006, SC2086
  3. mainly change whole image name (useful when you changing ECR)

@forevermatt
Copy link
Contributor

Hi @jindrichskupa. Thanks for your PR. However, it looks like some of the tests fail with those changes. Could you run them locally and do any necessary fixes?

docker-compose run --rm test

Just FYI, ecs-deploy is basically in maintenance mode, so only bug fixes and new aws CLI features will merit changes to this repo.

@v-stickykeys
Copy link
Contributor

@forevermatt would really like to see this merged. Also I ran the tests locally and see no failures. Can you share what failures you found?

@forevermatt
Copy link
Contributor

When I check out cookielab:cookie/change-whole-image-url and run the tests, tests 31 and 33 fail for me:

$ docker-compose run --rm test
Creating ecs-deploy_test_run ... done
fetch http://dl-3.alpinelinux.org/alpine/edge/community/x86_64/APKINDEX.tar.gz
fetch https://dl-cdn.alpinelinux.org/alpine/v3.13/main/x86_64/APKINDEX.tar.gz
fetch https://dl-cdn.alpinelinux.org/alpine/v3.13/community/x86_64/APKINDEX.tar.gz
(1/1) Installing bats (1.2.1-r0)
Executing busybox-1.32.1-r7.trigger
OK: 79 MiB in 58 packages
1..35
ok 1 check that usage() returns string and exits with status code 20
ok 2 test assertRequiredArgumentsSet success
ok 3 test assertRequiredArgumentsSet status=5
ok 4 test assertRequiredArgumentsSet status=6
ok 5 test assertRequiredArgumentsSet status=7
ok 6 test assertRequiredArgumentsSet status=8
ok 7 test assertRequiredArgumentsSet status=9
ok 8 test parseImageName missing image name
ok 9 test parseImageName invalid image name 1
ok 10 test parseImageName invalid port
ok 11 test parseImageName root image no tag
ok 12 test parseImageName root image with tag
ok 13 test parseImageName repo image no tag
ok 14 test parseImageName repo image with tag
ok 15 test parseImageName repo multilevel image no tag
ok 16 test parseImageName repo multilevel image with tag
ok 17 test parseImageName domain plus repo image no tag
ok 18 test parseImageName domain plus repo image with tag
ok 19 test parseImageName domain plus repo multilevel image no tag
ok 20 test parseImageName domain plus repo multilevel image with tag
ok 21 test parseImageName domain plus port plus repo image no tag
ok 22 test parseImageName domain plus port plus repo image with tag
ok 23 test parseImageName domain plus port plus repo multilevel image no tag
ok 24 test parseImageName domain plus port plus repo multilevel image with tag
ok 25 test parseImageName domain plus port plus repo image with tag from var
ok 26 test parseImageName domain plus port plus repo multilevel image with tag from var
ok 27 test parseImageName using ecr style domain
ok 28 test parseImageName using ecr style image name and tag from var
ok 29 test createNewTaskDefJson with single container in definition
ok 30 test createNewTaskDefJson with single container in definition for AWS Fargate
not ok 31 test createNewTaskDefJson with multiple containers in definition
# (in test file test.bats, line 432)
#   `[ "$(echo "$output" | jq .)" == "$(echo "$expected" | jq .)" ]' failed
ok 32 test createNewTaskDefJson with single container in definition and different repository
not ok 33 test createNewTaskDefJson with multiple containers in definition and different repository
# (in test file test.bats, line 573)
#   `[ "$(echo "$output" | jq .)" == "$(echo "$expected" | jq .)" ]' failed
ok 34 test parseImageName with tagonly option
ok 35 test createNewTaskDefJson with multiple containers in definition and replace only tags
ERROR: 1

@forevermatt
Copy link
Contributor

@v-stickykeys, if you don't mind looking into those failing tests and figuring out if it's truly a breaking change vs. outdated test expectations, I would welcome the help.

@devon-sil
Copy link
Contributor

Given the age of this PR and that it has conflicts, I'm going to close it out. If you still want to pursue this, please submit a new PR based on the latest code.

@devon-sil devon-sil closed this Jan 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants