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

[eas-cli] Only create a channel with branch name when branch is created #1507

Merged
merged 4 commits into from
Nov 23, 2022

Conversation

byCedric
Copy link
Member

@byCedric byCedric commented Nov 7, 2022

Checklist

  • I've added an entry to CHANGELOG.md if necessary. You can comment this pull request with /changelog-entry [breaking-change|new-feature|bug-fix|chore] [message] and CHANGELOG.md will be updated automatically.

Why

I noticed that whenever you have your own custom branch/channels mapping set up, it still created a new channel with that same branch name. Even when it's not necessary. This change was introduced in PR #1478, while trying to better surface important behavior within the command (instead of obfuscating it in side utility functions)

How

Preferably, I would implement something like:

const existingBranch = BranchQuery.getBranchByName({ name });
if (!existingBranch) {
  // create channel
}

But we throw errors in that case, which makes implementing this a bit harder. Instead, I opted for a isCreated boolean in the ensureBranchExists utility class.

Test Plan

Please describe how you tested this change and how a reviewer could reproduce your test, especially if this PR does not include automated tests! If possible, please also provide terminal output and/or screenshots demonstrating your test/reproduction.

@byCedric byCedric removed the request for review from wschurman November 7, 2022 13:42
@byCedric byCedric force-pushed the @bycedric/update/fix-channel-creation branch from ccb446b to 56871ac Compare November 7, 2022 15:05
@github-actions
Copy link

github-actions bot commented Nov 7, 2022

Size Change: -2.89 kB (0%)

Total Size: 40.3 MB

Filename Size Change
./packages/eas-cli/dist/eas-linux-x64.tar.gz 40.3 MB -2.89 kB (0%)

compressed-size-action

@codecov
Copy link

codecov bot commented Nov 7, 2022

Codecov Report

Merging #1507 (4c7886c) into main (597e012) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

❗ Current head 4c7886c differs from pull request most recent head 832f65c. Consider uploading reports for the commit 832f65c to get more accurate results

@@            Coverage Diff             @@
##             main    #1507      +/-   ##
==========================================
- Coverage   51.49%   51.49%   -0.00%     
==========================================
  Files         454      454              
  Lines       15675    15676       +1     
  Branches     3087     3088       +1     
==========================================
  Hits         8071     8071              
- Misses       7590     7591       +1     
  Partials       14       14              
Impacted Files Coverage Δ
packages/eas-cli/src/branch/queries.ts 22.96% <0.00%> (ø)
packages/eas-cli/src/commands/update/index.ts 14.91% <0.00%> (-0.05%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Contributor

@dsokal dsokal left a comment

Choose a reason for hiding this comment

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

Nit: branchIsCreated -> createdBranch. Maybe this is me but branchIsCreated should be always true after calling ensureBranchExistsAsync.

@byCedric
Copy link
Member Author

Nit: branchIsCreated -> createdBranch. Maybe this is me but branchIsCreated should be always true after calling ensureBranchExistsAsync.

Makes sense, done!

@byCedric
Copy link
Member Author

/changelog-entry bug-fix Only create a new channel when update branch is new

@byCedric byCedric merged commit d1bec6e into main Nov 23, 2022
@byCedric byCedric deleted the @bycedric/update/fix-channel-creation branch November 23, 2022 17:28
@kbrandwijk
Copy link
Contributor

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