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 support to customizing certbot cert command #2019

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

ZeekoZhu
Copy link

@ZeekoZhu ZeekoZhu commented Feb 26, 2024

Please see comments on #1761 for more details.

Most important items

  • Make sure to communicate your proposed changes ob our Slack channel beforehand.
  • Large PRs (50+ lines of code) will get rejected due to increased difficulty for review - unless they have been communicated beforehand with project maintainers.
  • Refactoring work will get rejected unless it's been communicated with project's maintainers beforehand. There is a thousand ways to write the same code. Every time a code is changed, there is a potential for a new bug. We don't want to refactor the code just for the sake of refactoring.

These rules are strictly enforced to make sure that we can maintain the project moving forward.

- Added getCertbotCertCommand method in CertbotManager for dynamic cert commands
- Refactored enableSsl method to use the new dynamic cert command generator
- Introduced certbotCertCommand configuration in CaptainConstants for customizable commands
- Extracted certificate success logic into isCertCommandSuccess function
- Ensured compatibility with previous certbot success messages
- Removed a redundant comma in function arguments
- Added a space before function declaration for consistency
- Introduced CertbotCertCommandRule for dynamic command generation
- Refactored CertbotManager to utilize a CertCommandGenerator
- Added new tests for the CertCommandGenerator logic
@ZeekoZhu
Copy link
Author

@githubsaturn Could you please let me know when you might have time to review the latest pull request? I appreciate your feedback on it.

Copy link
Collaborator

@githubsaturn githubsaturn left a comment

Choose a reason for hiding this comment

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

Shoot! So sorry for the late review!!


certbotImageName: 'caprover/certbot-sleeping:v1.6.0',

certbotCertCommand: undefined as CertbotCertCommandRule[] | undefined,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rename certbotCertCommand to certbotCertCommandRules

/**
* The Certbot command to execute, in Docker exec form, uses `${domain}` as the placeholder for the actual domain name
*/
command?: string[];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's use command?: string, when passing to exec, we can do command.trim().split(' ')

This will make it much easier to override and read.

}
return this.defaultCommand
}
getCertbotCertCommand(domainName: string, variables: Record<string, string> = {}): string[] {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The only place this command gets called we have:

                const cmd = self.certCommandGenerator.getCertbotCertCommand(domainName, {
                    domain: domainName,
                    webroot: WEBROOT_PATH_IN_CERTBOT + '/' + domainName
                })

Looks like the flexibility in the args is really not needed and causes more confusion. Let's just change the signature to be explicit. i.e.,

    getCertbotCertCommand(domainName: string, webroot:string)

@@ -12,8 +12,36 @@ const WEBROOT_PATH_IN_CAPTAIN =

const shouldUseStaging = false // CaptainConstants.isDebug;

function isCertCommandSuccess(output: string) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice!

@githubsaturn
Copy link
Collaborator

Hi @ZeekoZhu
Do you think you'll have time to update the PR? If not, I'll merge it as is and will update the changes later

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants