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

CLOUDP-273211: improve error handling and documentation for postman collection generation #252

Merged
merged 2 commits into from
Oct 11, 2024

Conversation

matt-condon
Copy link
Collaborator

@matt-condon matt-condon commented Oct 11, 2024

Proposed changes

Jira ticket: CLOUDP-273211

Small improvements to readme and logging to help with identifying potential issues in the postman script runs
Updated env vars and toggles

Checklist

  • I have signed the MongoDB CLA
  • I have added tests that prove my fix is effective or that my feature works

Further comments

@matt-condon matt-condon requested a review from a team as a code owner October 11, 2024 08:55
else \
echo "Error: $(ENV_FILE) file not found."; \
exit 1; \
fi
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added for local setup of env vars from local.env file

Copy link
Member

Choose a reason for hiding this comment

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

can this be done by

source local.env ?

Side note. As we discussed before, I think we have overgrown bash use cases.
If we continued working on postman, it would be good to use some scripting language.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was looking to set something simple up for the rare case where we have to do local validation

source local.env won't work in the makefile

I was thinking creating a separate script setting env vars was overkill when I could just put it in the Makefile since it'll probably be single use (just a little helper).

it would be good to use some scripting language

You mean an alternative scripting language?
I don't see us doing much more work on Postman barring very occasional patches to the post-processing or doc updates

iirc we were considering using TS originally for the post-processing, but there were limitations since we'd need Bazel set up, so bash was good enough (right tool for the job on this occasion)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If you feel strongly I'm happy to move it to a script (or remove it altogether since we can just set the small number of env vars up manually)
Thinking here was at least you can see the env vars in your local if you come back to do local validation another time

Copy link
Member

Choose a reason for hiding this comment

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

source local.env won't work in the makefile.

Yes but having logic for credentials processing in makefile is not best as well.
Typically make file calls single script.

I never have strong opinions. Only flagging some common pattern. Approved PR.

if [ "$TOKEN_URL_ENV" != "" ]; then
echo "Adding client credentials auth url variable $TOKEN_URL_ENV"
jq --arg token_url "$TOKEN_URL_ENV" '.collection.variable += [{"key": "clientCredentialsTokenUrl", "value": $token_url}]' \
intermediateCollectionPostBody.json > "$COLLECTION_TRANSFORMED_FILE_NAME"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added to set client credentials env var once available

jq --arg base_url "$BASE_URL" \
'.collection.variable[0].value = $base_url' \
intermediateCollectionWithDescription.json > intermediateCollectionWithBaseURL.json

echo "Adding links to docs"
cp intermediateCollectionWithBaseURL.json intermediateCollectionWithLinks.json
if [ "$TOGGLE_ADD_DOCS_LINKS" = "true" ]; then
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Created this toggle for local testing as this part of the script is long-running

@@ -23,16 +24,17 @@ OPENAPI_FOLDER=${OPENAPI_FOLDER:-"../openapi"}
TMP_FOLDER=${TMP_FOLDER:-"../tmp"}
VERSION_FILE_NAME=${VERSION_FILE_NAME:-"version.txt"}

DESCRIPTION_FILE=${DESCRIPTION_FILE:-"../collection-description.md"}

TOGGLE_USE_ENVIRONMENT_AUTH=${TOGGLE_USE_ENVIRONMENT_AUTH:-true}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed this one as we will always use env var auth

@@ -28,6 +28,9 @@ pushd "${OPENAPI_FOLDER}"
current_collection_name="MongoDB Atlas Administration API ${current_api_revision}"

echo "Fetching list of current collections"
echo "curl -o ${COLLECTIONS_LIST_FILE}
--location 'https://api.getpostman.com/collections?workspace=${WORKSPACE_ID}'
--header 'X-API-Key: **********'"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added logs to help with troubleshooting when observing CD runs

@@ -1,6 +1,7 @@
export OPENAPI_FOLDER=./openapi
export TMP_FOLDER=./tmp
export FULL_OPENAPI_FOLDER=../../openapi/v2/
ENV_FILE = ./local.env
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ENV_FILE = ./local.env
ENV_FILE = ./.env

Copy link
Member

Choose a reason for hiding this comment

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

Alternatively (if we want to export env vars):

Suggested change
ENV_FILE = ./local.env
ENV_FILE = ./env.sh

@wtrocki wtrocki closed this Oct 11, 2024
@wtrocki wtrocki reopened this Oct 11, 2024
Copy link
Member

@wtrocki wtrocki left a comment

Choose a reason for hiding this comment

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

LGTM, althought I would strongly advise to add gitignore to the credentials env file.

@matt-condon
Copy link
Collaborator Author

@wtrocki it's already covered in the existing gitignore

*.env

@matt-condon matt-condon merged commit af82bac into main Oct 11, 2024
9 checks passed
@matt-condon matt-condon deleted the CLOUDP-273211 branch October 11, 2024 15:20
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.

2 participants