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

[No QA] Simplify StagingDeployCash comparison URL #2261

Merged
merged 4 commits into from
Apr 8, 2021

Conversation

roryabraham
Copy link
Contributor

Details

I feel like a big dum-dum for not seeing this simplification earlier! Silver lining though: the contributor who worked on the generateVersionComparisonURL did set up a good baseline for unit testing the GithubUtils

Fixed Issues

Fixes https://github.com/Expensify/Expensify/issues/158542

Tests

  1. Make sure the StagingDeployCash is not locked.
  2. Merge this PR
  3. The StagingDeployCash should be updated and the "compare changes" link should be this: production...staging

Platforms

Github only, no changes in-product.

@roryabraham roryabraham requested review from AndrewGable and Jag96 April 7, 2021 06:25
@roryabraham roryabraham requested a review from a team as a code owner April 7, 2021 06:25
@roryabraham roryabraham self-assigned this Apr 7, 2021
@MelvinBot MelvinBot requested review from deetergp and removed request for a team April 7, 2021 06:26
Jag96
Jag96 previously approved these changes Apr 7, 2021
Copy link
Contributor

@Jag96 Jag96 left a comment

Choose a reason for hiding this comment

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

Nice improvement! Just one comment on a warning but LGTM

return issueBody;
})
// eslint-disable-next-line no-console
.catch(err => console.warn('Error generating comparison URL, continuing...', err));
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this warning still accurate? It sounds like this should be something like Error generating issue body

deetergp
deetergp previously approved these changes Apr 7, 2021
Copy link
Contributor

@deetergp deetergp left a comment

Choose a reason for hiding this comment

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

I agree with Joe regarding the error message, but other than that, it looks good.

@roryabraham roryabraham dismissed stale reviews from deetergp and Jag96 via b0d9b81 April 7, 2021 19:22
@roryabraham
Copy link
Contributor Author

Updated the console warning 👍

@roryabraham roryabraham requested a review from Jag96 April 7, 2021 19:22
@roryabraham roryabraham merged commit e2b96ef into master Apr 8, 2021
@roryabraham roryabraham deleted the Rory-SimplifyComparisonURL branch April 8, 2021 00:06
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.

3 participants