Skip to content
This repository has been archived by the owner on Sep 10, 2024. It is now read-only.

Adds error output to ssl_script/index.js #5

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

DougReeder
Copy link

No description provided.

@DougReeder DougReeder changed the title Notes that gen-hcce may need to be run a second time. Adds error output to ssl_script/index.js Sep 6, 2024
Copy link
Owner

@hrithikwins hrithikwins left a comment

Choose a reason for hiding this comment

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

The error messages and try catch for different scenarios look good
cc: @Exairnous

@Exairnous
Copy link
Contributor

@hrithikwins I guess this does provide a little extra information if something goes wrong, but it also changes the enclosure of the try/catch block for the certbotbot status call in the while loop which I don't think we should do. So I think I would accept this part of the PR if the try/catch enclosure is returned to its current coverage.

Also, before recommending to the user to just rerun the command if the namespace isn't found, I would want to know why the namespace isn't being found because I don't think that should happen. So I don't think this change should be included without more investigation into the issue.

@Exairnous
Copy link
Contributor

Also, perhaps it would make more sense to wait until the Node changes are merged into the hubs-cloud repository and then open up a new PR on that?

@DougReeder
Copy link
Author

In both the old and new code, errors thrown by execSync are caught and the process exited. The new code just brings the comparisons and logging inside the try-catch. In the unlikely event that they threw an error, that would almost certainly be a coding error, in which case the process should exit. However, that is so unlikely that practically speaking, the new and old code are equivalent.

IMNSHO, it comes down to whichever code is easier to comprehend. I think the new code is more idiomatic, but whichever way people want to do it is fine.

Copy link
Contributor

@Exairnous Exairnous left a comment

Choose a reason for hiding this comment

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

@DougReeder I'm not super familiar with javascript's idioms, but a lot of things I've read about programming have suggested that it's better to be explicit and unambiguous. However, you are right that this is only a minor change in scope, that it's unlikely there will be any coding errors in there, and since the code is directly related to the kubectl command, it's probably fine. I'm probably being overzealous here, so if @hrithikwins is good with the try/catch the way it is, then I will approve it too.

I looked some more into how kubernetes namespaces are created and I'm halfway changing my mind about the additional warning in the readme, but I've left a comment with an expanded explanation to help people understand what is likely going on. If this sounds correct to you, then I think this additional detail would be useful to people.

Co-authored-by: Exairnous <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants