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

DRIVERS-2497 authaws and ocsp helpers #246

Closed
wants to merge 2 commits into from

Conversation

rozza
Copy link
Contributor

@rozza rozza commented Nov 18, 2022

This PR is a followup to #236 and #244. Adds hygienic alternatives to Python virtual environment scripts for auth_aws and ocsp.

Tested against the Java driver in this patch with the following evergreen changes.


Notes

  1. I added reviewers for all those that participated in the previous patches - please feel free to remove yourself if this PR is not relevant to you.

  2. I had to update the shell in evergreen to explicitly use bash to get either:

. ./activate-mkstlsvenv.shand . ./activate-authawsvenv.sh to work.

Otherwise I got this error:

 ./activate-authawsvenv.sh: Syntax error: "(" unexpected (expecting "fi") 

Replace usage of .evergreen/auth_aws/
   - activate_venv.sh with
   - activate-authawsvenv.sh

DRIVERS-2497
For a consistent way to manage the ocsp venv

DRIVERS-2497
Copy link
Contributor

@eramongodb eramongodb left a comment

Choose a reason for hiding this comment

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

LGTM.

I had to update the shell in evergreen to explicitly use bash

As noted in DRIVERS-2497, Bash is required for the new scripts. According to documentation, the default shell used by Evergreen if not specified by a shell parameter is sh (Bourne Shell), which seems to often be confused with Bash or an alias for Bash. The fact that sh rather than bash is the default shell on Evergreen seems to be a reoccurring surprise.

I am in favor of the omission of inapplicable workarounds such as for greenlet and cryptography. I hope that they will no longer be necessary someday.

The repetition of similar script patterns across the three activate-* scripts makes me wonder if the required packages should have been specified as arguments to a common activation script, but alas.

@benjirewis benjirewis removed their request for review November 18, 2022 17:31
Copy link
Contributor

@rcsanchez97 rcsanchez97 left a comment

Choose a reason for hiding this comment

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

LGTM

@ShaneHarvey ShaneHarvey removed their request for review November 18, 2022 22:55
Copy link
Contributor

@kevinAlbs kevinAlbs left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you for the additions.

Consider adding a comment to the non-hygienic scripts to help discovery of the new scripts, e.g.

This script is deprecated. Use activate-authawsvenv.sh for a hygienic alternative.

.evergreen/auth_aws/activate-authawsvenv.sh Show resolved Hide resolved
.evergreen/ocsp/activate-ocspvenv.sh Show resolved Hide resolved
@rozza rozza closed this Nov 21, 2022
@rozza
Copy link
Contributor Author

rozza commented Nov 21, 2022

Manually merged and closed.

@rozza rozza deleted the DRIVERS-2497 branch November 21, 2022 10:05
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

4 participants