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.

Sign up for GitHub

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

Jump to bottom

feat(cli): use requestedUrl instead of finalUrl for upload context naming #849

Merged
merged 3 commits into from
Feb 21, 2023
Merged

feat(cli): use requestedUrl instead of finalUrl for upload context naming #849

merged 3 commits into from
Feb 21, 2023

Conversation

jaspritt-hora
Copy link
Contributor

CHANGED:

  • Using requestedUrl instead of finalUrl for runFilesystemTarget function.
  • Updated the test cases for target=filesystem in upload.test.js
  • Updated the test cases for upload-url-hash.test.js

SUPPORTING INFO:

@jaspritt-hora
Copy link
Contributor Author

jaspritt-hora commented Dec 5, 2022 •
edited
Loading

@connorjclark / @patrickhulce - Please review the changes. I have made changes wrt target=filesystem only for now.

FYI! The yarn test failure for windows & mac is NOT because of the test cases changed for this PR. The test case files for this PR have successfully passed in all the three environments.

patrickhulce
patrickhulce reviewed Dec 14, 2022
View reviewed changes
Copy link
Collaborator

@patrickhulce patrickhulce left a comment •
edited
Loading

Choose a reason for hiding this comment

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

technically breaking but seems fine to me to slip in a minor 🤷

connorjclark
connorjclark reviewed Feb 15, 2023
View reviewed changes
packages/cli/test/upload-url-hash.test.js Outdated
@@ -1,5 +1,5 @@
/**
* @license Copyright 2019 Google Inc. All Rights Reserved.
Copy link
Collaborator

Choose a reason for hiding this comment

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

please revert - as a policy we don't update these years

Copy link
Contributor Author

@jaspritt-hora jaspritt-hora Feb 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@connorjclark - Done and I have squashed the commits as well. Pipeline failing for flaky tests. The test file in the PR have passed on all three OS.

@connorjclark connorjclark changed the title feat(cli): Updated runFilesystemTarget fn to use requestedUrl feat(cli): use requestedUrl instead of finalUrl for upload context naming Feb 21, 2023
@connorjclark connorjclark merged commit 2618b5f into GoogleChrome:main Feb 21, 2023
@connorjclark
Copy link
Collaborator

Thanks @jaspritt-hora!

@kingjay66 kingjay66 mentioned this pull request Oct 14, 2023
Open
@MaxMood96 MaxMood96 mentioned this pull request Nov 27, 2023
Open
This was referenced Nov 27, 2023
Open
Open
@kingjay66 kingjay66 mentioned this pull request Nov 29, 2023
Open
@NOUIY NOUIY mentioned this pull request May 26, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@patrickhulce patrickhulce patrickhulce left review comments

@connorjclark connorjclark Awaiting requested review from connorjclark

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jaspritt-hora @connorjclark @patrickhulce