Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 HTTPS to Sidecar for deployment #8
Add HTTPS to Sidecar for deployment #8
Changes from 47 commits
51d13c5
ff09d45
512dd30
ce75e4c
01eab14
da0e501
9ad2140
c23b5cd
f9fa873
45512b5
49b785b
e087366
6459150
4cfac07
6eb0be1
05416d5
1ac44f7
6aed89e
89458c4
bffedef
48684ff
d3e7324
a303c0c
2f7e64a
7e8c5d8
ec40bcb
f40f955
6ead567
9273e54
bc772fc
0a87b6d
5956af0
609a16e
656a7e4
136d5be
9660d9f
a093803
cfecf75
f727f47
d5873a8
f815f57
66ccf83
0dc32d8
1153d1d
2dfb717
3103593
37984f0
896820a
6db365a
180abe2
158d810
591d4b0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The instructions for generating TLS certificates with Let's Encrypt are well-detailed. However, there's a potential security concern with the use of
sudo
in the command for runningcertbot
. It's recommended to use a system-wide installation ofcertbot
rather than invoking it through the virtual environment withsudo
. This approach reduces the risk associated with elevated privileges and ensures thatcertbot
operates in a more controlled environment.Committable suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The TODO under "Generate IPA specific keys" is left unaddressed in the README. It's important to either complete this section with the necessary instructions or remove the placeholder if it's not applicable. Leaving TODOs in documentation can lead to confusion for users trying to follow the setup instructions.
Would you like assistance in completing this section, or should it be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a warning for when the GitHub API key is not set is a good practice for clarity and debugging. However, consider also implementing a more robust error handling mechanism for when the API key is expired, as mentioned in the TODO comment. This could involve catching specific exceptions related to API key expiration and providing clear error messages or recovery steps.
Would you like assistance in implementing error handling for expired API keys?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes please provide a suggestion. note that the key doesn't require any permissions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eriktaubeneck, considering the Octokit client's usage in your application and the requirement that the API key does not need any permissions, you can enhance error handling by wrapping your API calls in a try-catch block. This approach allows you to catch any errors thrown by Octokit, including those related to expired or invalid API keys. Here's a suggestion for how you might implement this:
This example wraps the
fetchBranches
function (a hypothetical function for fetching branches, as an example) in a try-catch block. It specifically checks for a 401 status code, which is commonly used to indicate authentication errors, including expired or invalid API keys. Adjust the error handling as necessary for your application's needs, including the possibility of re-throwing the error or handling different types of errors differently.Remember to apply a similar pattern to other Octokit API calls in your application to ensure consistent and robust error handling across your application.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic to prioritize the main branch by moving it to the front of the branches array is a practical approach for repositories where the main branch is of primary interest. However, adding a placeholder branch with name "N/A" and empty commit hash at the beginning of the array might lead to confusion or errors in downstream processing. Consider documenting this behavior or revising the approach to ensure it aligns with the expectations of consumers of the
Branches
function.