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

Perform direct asset name comparison #102

Merged
merged 5 commits into from
Jan 24, 2022
Merged

Perform direct asset name comparison #102

merged 5 commits into from
Jan 24, 2022

Conversation

tonerdo
Copy link
Contributor

@tonerdo tonerdo commented Jan 15, 2022

Description

Handle situations where the release asset name contains regex symbols that could mess up matching.

Documentation

TODOs

Please ensure all of these TODOs are completed before asking for a review.

  • Ensure the branch is named correctly with the issue number. e.g: feature/new-vpc-endpoints-955 or bug/missing-count-param-434.
  • Update the docs.
  • Keep the changes backward compatible where possible.
  • Run the pre-commit checks successfully.
  • Run the relevant tests successfully.
  • Ensure any 3rd party code adheres with our license policy or delete this line if its not applicable.

Related Issues

Fixes #93

@rhoboat
Copy link
Contributor

rhoboat commented Jan 15, 2022

I kicked off the build and it passed on CI!

Copy link
Member

@brikis98 brikis98 left a comment

Choose a reason for hiding this comment

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

Thx for the PR! Could you add some testing around this so it doesn't break again in the future? Would be good to test with various names too as a check this really fixes the issue.

@tonerdo
Copy link
Contributor Author

tonerdo commented Jan 18, 2022

@brikis98 is there a repo where it'll be okay to upload multiple release assets to help with testing? I don't want to pollute any of our actual repos.

@rhoboat
Copy link
Contributor

rhoboat commented Jan 19, 2022

It would make sense to use these:
https://github.com/gruntwork-io/fetch-test-public
https://github.com/gruntwork-io/fetch-test-private

Do you have access to these @tonerdo ? (credit to @yorinasub17)

@tonerdo
Copy link
Contributor Author

tonerdo commented Jan 20, 2022

@rhoboat I don't have access to create new releases on https://github.com/gruntwork-io/fetch-test-public. I don't have access to https://github.com/gruntwork-io/fetch-test-private at all

@tonerdo
Copy link
Contributor Author

tonerdo commented Jan 23, 2022

@brikis98 I was only able to add a test for an asset with the + sign. Good thing is that when I try to upload files containing other regex characters, GitHub simply converts them to ..

@tonerdo tonerdo merged commit 625331e into master Jan 24, 2022
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.

Fetching asset files with + signs causes them not to be found.
4 participants