CM-318: Add case to use explicit credentials in ACME DNS-01 on GCP#181
Conversation
|
@lunarwhite: This pull request references CM-318 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.16.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
Skipping CI for Draft Pull Request. |
|
/test e2e-operator |
|
Async |
|
/lgtm |
|
@lunarwhite: This pull request references CM-318 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.16.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
/test fips-image-scan |
|
@swghosh @TrilokGeer help approve when you have a chance :) |
|
/test fips-image-scan |
|
@lunarwhite: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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-sigs/prow repository. I understand the commands that are listed here. |
|
/lgtm |
|
/label docs-approved |
test/e2e/certificates_test.go
Outdated
| By("getting Infrastructure object to check plaform type") | ||
| infra, err := configClient.Infrastructures().Get(ctx, "cluster", metav1.GetOptions{}) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| if infra.Status.PlatformStatus.Type != configv1.GCPPlatformType { |
There was a problem hiding this comment.
Using a util function would help platform type differentiation.
test/e2e/certificates_test.go
Outdated
| defer loader.DeleteTestingNS(ns.Name) | ||
|
|
||
| By("obtaining GCP credentials from kube-system namespace") | ||
| gcpCredsSecret, err := loader.KubeClient.CoreV1().Secrets("kube-system").Get(ctx, "gcp-credentials", metav1.GetOptions{}) |
There was a problem hiding this comment.
Can we generalize platform-specific requests to beforesuite() ? This'd help to fail the suite early in case of access issues with platforms than iterating the testcases.
There was a problem hiding this comment.
+1 we can filter out basis of platform on BeforeSuite or somewhere early.
I had a PoC with use of label filters that I was experimenting in #192 we can use similar if that makes sense. Also, considering what is in #192, I used two different Test* go func(s) to run separate suites that ginkgo doesn't generally avoid and while refactoring it we should ensure to not use that. Our Makefile can use a environment var for platform = AWS / GCP / etc. which ginkgo can filter via labels, if that sounds reasonable.
|
@lunarwhite: This pull request references CM-318 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.17.0" version, but no target version was set. This pull request references CM-261 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.17.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
swghosh
left a comment
There was a problem hiding this comment.
I got the aim for this PR
(being: AWS has both explicit, implicit creds issuer, GCP lacks explicit creds; supporting completeness of tests),
but it seems to me that it'd be worth to re-work this PR a bit on the basis of #194 (once t'is merged).
It'd help in adding the Platform:GCP label to this specific test and avoid unnecessary rebase across conflict too. Hope that works!
|
generally looking fine on the contents of the test case, |
|
@lunarwhite: This pull request references CM-318 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the sub-task to target the "4.18.0" version, but no target version was set. This pull request references CM-261 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.18.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
@lunarwhite: This pull request references CM-318 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the sub-task to target the "4.18.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
@swghosh Ready for another review. You could take a look when you're around. TYIA! |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lunarwhite, swghosh, xingxingxia The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/remove-hold |
Configuring an ACME issuer by using explicit credentials for GCP CloudDNS
Similair to "should obtain a valid LetsEncrypt certificate", but change to use CloudDNS DNS-01 solver
Pass log: