Skip to content

Conversation

@FGasper
Copy link
Collaborator

@FGasper FGasper commented Sep 4, 2025

This facilitates use in CI.

Copy link
Collaborator

@autarch autarch left a comment

Choose a reason for hiding this comment

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

I think there's a simpler way to do this.


echo "Looks like you’re running $OS on $ARCH."

MANIFEST=$(wget -qO- "$RELEASE_URL")
Copy link
Collaborator

Choose a reason for hiding this comment

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

The instructions tell people to use curl to fetch this script, but then it uses wget internally. I think it should curl since we know they already have that installed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But maybe this isn't needed at all.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Switched to curl.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is more complex than it needs to be. Once you know the filename you want to download, you can download this: https://github.com/mongodb-labs/migration-verifier/releases/latest/download/$FILENAME

There's no need to hit the API and parse the response.

This does mean you need to calculate the filename directly instead of getting it from the API response, but it seems a lot simpler. You can see how I do this for ubi at https://github.com/houseabsolute/ubi/blob/master/bootstrap/bootstrap-ubi.sh

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’d rather not build in a dependency on the filename format.

OK to keep as-is?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure, that's fine. But I'd note that we do control the filename!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You know that & I know that, but the next maintainer may be a new hire or intern.

@FGasper FGasper requested a review from autarch September 5, 2025 13:55
Copy link
Collaborator

@autarch autarch left a comment

Choose a reason for hiding this comment

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

LGTM!

@FGasper FGasper merged commit ee2106a into mongodb-labs:main Sep 5, 2025
98 checks passed
@FGasper FGasper deleted the felipe_add_download_script branch September 5, 2025 15:15
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