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

test/s3: keep the bucket name length under control (<63 characters) #340

Conversation

goneri
Copy link
Member

@goneri goneri commented Apr 26, 2021

The bucket_name variable length is 63 or below. If we add a prefix we
can generate an invalid bucket name.

See: https://docs.aws.amazon.com/AmazonS3/latest/userguide/bucketnamingrules.html

@ansibullbot
Copy link

@goneri goneri requested a review from jillr April 26, 2021 15:31
@jillr
Copy link
Collaborator

jillr commented Apr 26, 2021

While it's probably safe to drop these tests, I'd generally prefer not to drop tests if possible. We'll probably also see resource name length issues in other test suites, are we not able to control the names we get from zuul?

@goneri goneri force-pushed the aws_s3-do-not-test-bucket-name-with-a-dot_23689 branch from db7eca6 to a78d78f Compare April 26, 2021 16:44
The bucket_name variable length is 63 or below. If we add a prefix we
can generate an invalid bucket name.

In addition, the test has little value since it just ensures end-point
behaves as expected. The code coverage in the module itself remains
the same.

See: https://docs.aws.amazon.com/AmazonS3/latest/userguide/bucketnamingrules.html
@goneri goneri force-pushed the aws_s3-do-not-test-bucket-name-with-a-dot_23689 branch from a78d78f to 6011499 Compare April 26, 2021 16:52
@goneri
Copy link
Member Author

goneri commented Apr 26, 2021

While it's probably safe to drop these tests, I'd generally prefer not to drop tests if possible. We'll probably also see resource name length issues in other test suites, are we not able to control the names we get from zuul?

The prefix is generated by ansible-test. Follow up to this two PR what was a de facto convention is now enforced. The prefix is 63 character long.

So we're good as soon as we don't had anything to the prefix.

Regarding this specific case, I was under the impression the test is a bit pointless. I'm since updated my patch to continue to run it. I now build the bucket name with the MD5 of the prefix.

@goneri goneri changed the title aws_s3: do not test bucket name with a dot test/s3: keep the bucket name length under control (<63 characters) Apr 26, 2021
@jillr
Copy link
Collaborator

jillr commented Apr 26, 2021

The test might be pointless, it seems likely. When it was written the module was boto2 so it's entirely possible it was addressing some case that's no longer a concern. But I couldn't find any more info about why we wrote that test, like if there's a history of S3/the SDK doing weird things with those bucket names. So I'm hesitant to remove the tests just to be conservative.

@goneri
Copy link
Member Author

goneri commented Apr 26, 2021

The test might be pointless, it seems likely. When it was written the module was boto2 so it's entirely possible it was addressing some case that's no longer a concern. But I couldn't find any more info about why we wrote that test, like if there's a history of S3/the SDK doing weird things with those bucket names. So I'm hesitant to remove the tests just to be conservative.

Sorry for the harsh tone of my initial comment :-). I started a PR to validate the name of the bucket: #341

@jillr
Copy link
Collaborator

jillr commented Apr 26, 2021

No worries, no hard tone was heard! :)

@goneri goneri added the gate label Apr 26, 2021
@ansible-zuul ansible-zuul bot merged commit 26a9c73 into ansible-collections:main Apr 26, 2021
abikouo pushed a commit to abikouo/amazon.aws that referenced this pull request Sep 18, 2023
…ible-collections#340)

ecs_service - support setting deployment controller on a service

SUMMARY
Support setting platform version to 1.4.0 (LATEST is 1.3.0) and deployment controller.
The first allows access to new 1.4.0 features. The second change allows you to create a service that can be controlled with Code Deploy.
Example:
  - name: create a Fargate service
    community.aws.ecs_service:
      state: present
      name: "my-service"
      cluster: "my-cluster"
      platform_version: 1.4.0
      task_definition: "my-task"
      desired_count: "1"
      launch_type: FARGATE
      scheduling_strategy: REPLICA
      deployment_controller:
        type: CODE_DEPLOY
      load_balancers:
        - targetGroupArn: "arn:..."
          containerName: example
          containerPort: 80
      network_configuration:
        subnets:
          - "{{vpc_zone_a.subnet.id}}"
          - "{{vpc_zone_b.subnet.id}}"
        security_groups:
          - "sg-example"
        assign_public_ip: true

This fixes ansible-collections#338.
ISSUE TYPE

Feature Pull Request

Reviewed-by: Jill R <None>
Reviewed-by: Mark Chappell <None>
Reviewed-by: Markus Bergholz <[email protected]>
abikouo pushed a commit to abikouo/amazon.aws that referenced this pull request Sep 18, 2023
…ible-collections#340)

ecs_service - support setting deployment controller on a service

SUMMARY
Support setting platform version to 1.4.0 (LATEST is 1.3.0) and deployment controller.
The first allows access to new 1.4.0 features. The second change allows you to create a service that can be controlled with Code Deploy.
Example:
  - name: create a Fargate service
    community.aws.ecs_service:
      state: present
      name: "my-service"
      cluster: "my-cluster"
      platform_version: 1.4.0
      task_definition: "my-task"
      desired_count: "1"
      launch_type: FARGATE
      scheduling_strategy: REPLICA
      deployment_controller:
        type: CODE_DEPLOY
      load_balancers:
        - targetGroupArn: "arn:..."
          containerName: example
          containerPort: 80
      network_configuration:
        subnets:
          - "{{vpc_zone_a.subnet.id}}"
          - "{{vpc_zone_b.subnet.id}}"
        security_groups:
          - "sg-example"
        assign_public_ip: true

This fixes ansible-collections#338.
ISSUE TYPE

Feature Pull Request

Reviewed-by: Jill R <None>
Reviewed-by: Mark Chappell <None>
Reviewed-by: Markus Bergholz <[email protected]>
abikouo pushed a commit to abikouo/amazon.aws that referenced this pull request Oct 24, 2023
…ible-collections#340)

ecs_service - support setting deployment controller on a service

SUMMARY
Support setting platform version to 1.4.0 (LATEST is 1.3.0) and deployment controller.
The first allows access to new 1.4.0 features. The second change allows you to create a service that can be controlled with Code Deploy.
Example:
  - name: create a Fargate service
    community.aws.ecs_service:
      state: present
      name: "my-service"
      cluster: "my-cluster"
      platform_version: 1.4.0
      task_definition: "my-task"
      desired_count: "1"
      launch_type: FARGATE
      scheduling_strategy: REPLICA
      deployment_controller:
        type: CODE_DEPLOY
      load_balancers:
        - targetGroupArn: "arn:..."
          containerName: example
          containerPort: 80
      network_configuration:
        subnets:
          - "{{vpc_zone_a.subnet.id}}"
          - "{{vpc_zone_b.subnet.id}}"
        security_groups:
          - "sg-example"
        assign_public_ip: true

This fixes ansible-collections#338.
ISSUE TYPE

Feature Pull Request

Reviewed-by: Jill R <None>
Reviewed-by: Mark Chappell <None>
Reviewed-by: Markus Bergholz <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community_review integration tests/integration plugins plugin (any type) tests tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants