Skip to content

Improve test case #5

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Improve test case #5

wants to merge 8 commits into from

Conversation

cmoulliard
Copy link

  • Ignore idea and _out directory
  • Add missing dot end of the DNS name
  • Update dependencies. To be controlled
  • Uncomment email and ApiSecretRef
  • Add script to fetch kubebuilder binaries
  • Add DNS server. Set additional vars

cmoulliard added 6 commits May 29, 2019 10:43
@cmoulliard
Ignore idea and _out directory
6294ce1 
Signed-off-by: Charles Moulliard <[email protected]>
@cmoulliard
Add missing dot end of the DNS name
3bb6f6c 
Signed-off-by: Charles Moulliard <[email protected]>
@cmoulliard
Update dependencies. To be controlled
9b4aa49 
Signed-off-by: Charles Moulliard <[email protected]>
@cmoulliard
Uncomment email and ApiSecretRef
f96df3b 
Signed-off-by: Charles Moulliard <[email protected]>
@cmoulliard
Add script to fetch kubebuilder binaries
296fbde 
Signed-off-by: Charles Moulliard <[email protected]>
@cmoulliard
Add DNS server. Set additional vars
060ffb3 
Signed-off-by: Charles Moulliard <[email protected]>
@jetstack-bot jetstack-bot added the dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. label May 29, 2019
@jetstack-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: cmoulliard
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approvers:

If they are not already assigned, you can assign the PR to them by writing /assign in a comment when ready.

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

@jetstack-bot jetstack-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 29, 2019
@cmoulliard
Remove non used vars. Add setUseAuthoritative(false) to avoid the err…
7621c67 
…or : suite.go:49: error waiting for DNS record propagation: Could not determine authoritative nameservers for cert-manager-dns01-tests.example.com.
@jetstack-bot jetstack-bot added dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. and removed dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. labels May 29, 2019
@cmoulliard
Copy link
Author

/assign @munnerz

@cmoulliard
Copy link
Author

FYI. The link to the CONTRIBUTING guide is missing @munnerz -> https://github.com/jetstack/cert-manager-webhook-example/blob/master/CONTRIBUTING.md

@jetstack-bot
Copy link
Contributor

Thanks for your pull request. Before we can look at it, you'll need to add a 'DCO signoff' to your commits.

📝 Please follow instructions in the contributing guide to update your commits with the DCO

Full details of the Developer Certificate of Origin can be found at developercertificate.org.

The list of commits missing DCO signoff:

  • 7621c67 Remove non used vars. Add setUseAuthoritative(false) to avoid the error : suite.go:49: error waiting for DNS record propagation: Could not determine authoritative nameservers for cert-manager-dns01-tests.example.com.
  • 7415d9a Added local DNS server. Signed-off-by: Charles Moulliard [email protected]

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.

@jetstack-bot jetstack-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 3, 2019
@cmoulliard
Copy link
Author

@munnerz Can you have a look to this ticket ?

munnerz
munnerz reviewed Jul 9, 2019
View reviewed changes
Copy link
Member

@munnerz munnerz left a comment

Choose a reason for hiding this comment

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

Thanks for putting this together and sorry for the delay 😬

I've added a few comments - mostly I think we should keep the example fairly generic and not start talking about the RFC2136 issuer here, as it could cause confusion for users getting started writing their own webhook.

main.go
//Email string `json:"email"`
//APIKeySecretRef v1alpha1.SecretKeySelector `json:"apiKeySecretRef"`
Email string `json:"email"`
APIKeySecretRef certmanagerv1.SecretKeySelector `json:"apiKeySecretRef"`
Copy link
Member

Choose a reason for hiding this comment

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

Not all of these fields are required by all DNS provider types, hence keeping the contents here generic. Can you revert these changes, and if you feel there could be additional clarification as to what a user should do here then add comments to explain it?

Copy link
Author

Choose a reason for hiding this comment

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

I don't think that I included them or maybe this is because they were defined within one of the example of the cert manager project. So, do you suggest that we remove the fields Email and APIKeySecretRef ?

main_test.go

var validConfig = cmapi.ACMEIssuerDNS01ProviderRFC2136{
Nameserver: server.ListenAddr(),
}
Copy link
Member

Choose a reason for hiding this comment

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

Referencing the RFC2136 solver here is confusing to users, and also won't result in the RFC2136 issuer actually being used (as dns.NewFixture(&customDNSProviderSolver{} is still called).

Having the binaries path set + the script to fetch those binaries is great, but I think this example should be generic and show a basic setup that needs a bit of modification or commenting/uncommenting of example lines to get things to work.

Copy link
Author

Choose a reason for hiding this comment

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

So, what do you suggest that we do then here ?

@jetstack-bot jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 9, 2020
@jetstack-bot
Copy link
Contributor

@cmoulliard: PR needs rebase.

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.

@cert-manager-prow cert-manager-prow bot mentioned this pull request Apr 22, 2024
Closed
OliverOlg added a commit to OliverOlg/cert-manager-webhook-infoblox-wapi that referenced this pull request Mar 26, 2025
@OliverOlg
Squashed commit of the following:
7cd59da 
commit 1fae2e8
Author: Luis Gracia <[email protected]>
Date:   Wed Dec 18 09:52:36 2024 +1300

    Update README.md

    Add reference to new project

commit bb36abe
Author: Luis Gracia <[email protected]>
Date:   Wed Oct 2 16:11:41 2024 +1300

    Deprecate repo

commit ef886b6
Author: Luis Gracia <[email protected]>
Date:   Wed Apr 13 14:03:52 2022 -0400

    Update version

commit 03c5086
Merge: a09d627 040a813
Author: Luis Gracia <[email protected]>
Date:   Wed Apr 13 13:54:49 2022 -0400

    Merge pull request cert-manager#5 from ONordander/feature/configure-cert-duration

    Add possibility to configure cert duration

commit 040a813
Author: Otto Nordander <[email protected]>
Date:   Tue Apr 12 08:21:06 2022 +0200

    Remove webhook prefix for certificates

commit 10aee3f
Author: Otto Nordander <[email protected]>
Date:   Tue Apr 12 08:20:54 2022 +0200

    Add documentation of values.yaml

commit 823531f
Author: Otto Nordander <[email protected]>
Date:   Thu Apr 7 14:20:58 2022 +0200

    Add possibility to configure cert duration

commit a09d627
Merge: 55d0a22 75cb2b5
Author: Luis Gracia <[email protected]>
Date:   Tue Oct 12 14:47:43 2021 -0400

    Merge pull request cert-manager#3 from luisico/upgrade-alpine

    Upgrade alpine image for container

commit 75cb2b5
Author: Luis Gracia <[email protected]>
Date:   Tue Oct 12 14:46:17 2021 -0400

    Upgrade alpine image for container

    Solve security issues pointed out by artifacthub.

commit 55d0a22
Merge: 0e43959 95451b7
Author: Luis Gracia <[email protected]>
Date:   Tue Oct 12 13:11:25 2021 -0400

    Merge pull request cert-manager#2 from luisico/release-on-tags

    Release on tags

commit 95451b7
Author: Luis Gracia <[email protected]>
Date:   Tue Oct 12 13:08:36 2021 -0400

    Always push docker images

commit 5a75aa0
Author: Luis Gracia <[email protected]>
Date:   Tue Oct 12 10:31:25 2021 -0400

    Version sync with cert-manager

    Version scheme to keep major and minor version in sync with cert-manager

commit 3951e65
Author: Luis Gracia <[email protected]>
Date:   Tue Oct 12 12:31:52 2021 -0400

    Use luisico/chart-releaser-action@on-tags

    helm/chart-releaser-action does not support actions
    triggered by tags

commit dbec1b8
Author: Luis Gracia <[email protected]>
Date:   Mon Oct 11 19:09:35 2021 -0400

    Release docker images and helm charts on tags

commit 0360b99
Author: Luis Gracia <[email protected]>
Date:   Mon Oct 11 12:20:17 2021 -0400

    Release docker image

commit 3944741
Author: Luis Gracia <[email protected]>
Date:   Mon Oct 11 11:42:52 2021 -0400

    Release helm chart

commit 0e43959
Merge: 9440683 f977e2b
Author: Luis Gracia <[email protected]>
Date:   Tue Oct 12 12:51:03 2021 -0400

    Merge pull request cert-manager#1 from luisico/infoblox-wapi

    Infoblox wapi

commit f977e2b
Author: Luis Gracia <[email protected]>
Date:   Mon Oct 11 16:02:58 2021 -0400

    Add NOTICE file listing files changes from the original source code

commit 710488b
Author: Luis Gracia <[email protected]>
Date:   Thu Oct 7 11:30:14 2021 -0400

    Fix authorization for flowcontrol

commit af684d5
Author: Luis Gracia <[email protected]>
Date:   Mon Oct 11 14:05:08 2021 -0400

    Add namespaces

commit a8c56ef
Author: Luis Gracia <[email protected]>
Date:   Mon Oct 11 12:57:38 2021 -0400

    Add readme

commit c474d07
Author: Luis Gracia <[email protected]>
Date:   Fri Oct 8 12:21:50 2021 -0400

    Adatap directory structure and files to new package name

commit 203e7c4
Author: Luis Gracia <[email protected]>
Date:   Fri Oct 8 13:16:53 2021 -0400

    Refactor txt record deletion

commit 7d66a33
Author: Luis Gracia <[email protected]>
Date:   Thu Oct 7 19:01:07 2021 -0400

    Add structured logs adhering to kubernetes guidelines

    - https://github.com/kubernetes/community/blob/master/contributors/devel/sig-instrumentation/logging.md
    - https://github.com/kubernetes/community/blob/master/contributors/devel/sig-instrumentation/migration-to-structured-logging.md

commit eabd6df
Author: Luis Gracia <[email protected]>
Date:   Tue Oct 5 17:16:35 2021 -0400

    Initial adaptation for infoblox-wapi
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@munnerz munnerz munnerz left review comments

Assignees

@munnerz munnerz

Labels
dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cmoulliard @jetstack-bot @munnerz