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

Add CA immutable fields test #59

Merged
merged 6 commits into from
Oct 8, 2024

Conversation

ndbhat
Copy link
Contributor

@ndbhat ndbhat commented Jul 15, 2024

Description of changes:

  1. Adding test case to check that the CA resource recovers after immutable fields are updated

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@ack-prow ack-prow bot requested review from anbaig and divyansh-gupta July 15, 2024 14:35
Copy link

ack-prow bot commented Jul 15, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ndbhat

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:

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

@ack-prow ack-prow bot added the approved label Jul 15, 2024
@ndbhat ndbhat requested review from a-hilaly and removed request for divyansh-gupta July 15, 2024 14:35

acmpca_validator = ACMPCAValidator(acmpca_client)
ca = acmpca_validator.assert_certificate_authority(ca_resource_arn, "PENDING_CERTIFICATE")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also assert that the resource is in terminal state?

Comment on lines 522 to 523
# assert 'status' in ca_cr['status']
# assert ca_cr['status']['status'] == "ACTIVE"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why did these get commented out?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these lines started failing when I removed reconcile: requeue_on_success_seconds: 30 from generator.yaml, getting a fix out in another PR

Comment on lines 1010 to 1011
// Required to avoid the "declared but not used" error in the default case
_ = syncCondition
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

won't this just get overwritten the next time someone run generate?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this code was auto removed when I removed reconcile: requeue_on_success_seconds: 30 from generator.yaml

Comment on lines 30 to 31
reconcile:
requeue_on_success_seconds: 30
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why did this get removed?

My concern is that you create a CA and the state in ACK shows PENDING_CERTIFICATE for a veryyyy long time now despite the activation resource working successfully.

Copy link

ack-prow bot commented Aug 30, 2024

@ndbhat: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
acmpca-verify-attribution e8c7417 link true /test acmpca-verify-attribution

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@anbaig anbaig added the lgtm Indicates that a PR is ready to be merged. label Oct 8, 2024
@ack-prow ack-prow bot merged commit b556979 into aws-controllers-k8s:main Oct 8, 2024
6 of 7 checks passed
ack-prow bot pushed a commit that referenced this pull request Oct 8, 2024
Description of changes:
Release artifacts for release v0.0.19
Merge after #59 is merged

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
@ndbhat ndbhat deleted the immutable-field-test branch October 23, 2024 17:23
ndbhat added a commit to ndbhat/ack-acmpca-controller that referenced this pull request Oct 23, 2024
Description of changes:
1. Adding test case to check that the CA resource recovers after immutable fields are updated

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants