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] Extract entity utilities from update command class #1478

Merged
merged 8 commits into from
Oct 31, 2022

Conversation

byCedric
Copy link
Member

@byCedric byCedric commented Oct 25, 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

Continuation of PR #1473 and ENG-5926.

How

  • Moved ensure methods from UpdatePublish to src/branch|channel/queries.ts.
  • Copied other methods from commands BranchCreate and ChannelCreate, to keep queries contained in src/branch|channel/queries.ts files.
  • Validated the logging is similar (still slightly different)
    • branch logging
      • removed: Created branch: <branchName>
      • why: The branch is both listed in the formatted fields (first field), and the "Channel is pointing to branch" logging, seems not worth it to determine if the branch was created or already existed. (but if people disagree, I'd be happy to change it)
    • channel logging
      • from: Created a channel: <channelName> pointed at branch: <branchName> (only logged when created)
      • to: Channel: <channelName> pointed at branch: <branchName> (always logged)
      • why: The channel name is not part of the formatted fields at the end. We either need to display this, or maybe even add it to the formatted fields as item? (instead of the Log.withTick)

Test Plan

  • eas update --branch new-branch
  • eas update --branch existing-branch

@github-actions
Copy link

github-actions bot commented Oct 25, 2022

Size Change: -160 B (0%)

Total Size: 40.1 MB

Filename Size Change
./packages/eas-cli/dist/eas-linux-x64.tar.gz 40.1 MB -160 B (0%)

compressed-size-action

@byCedric
Copy link
Member Author

byCedric commented Oct 25, 2022

Open questions to everyone here:

  1. Do we want to keep exporting these create<Entity> or ensure<Entity>... methods from other command classes? I don't really like it, since it implicitly creates depending command classes. But, not sure if there is a reason to do it. If we ever want to make some commands load async, I think we can't have these command class dependencies.
  2. Why do we have src/graphql/queries|mutations and separate src/branch|channel/queries.ts files? Seems like they would be more in place under the graphql client, like most of the non-updates-related entities?
  3. Seeing some inconsistencies with the return types and logic of ensure<Entity> methods. Some won't return anything, others just the entity ID. The ensureChannelExists also yolos it and uses potential errors to determine if the channel exists, makes sense in terms of limiting the graphql calls, but it does differ quite a lot compared to ensureBranchExists.

@codecov
Copy link

codecov bot commented Oct 25, 2022

Codecov Report

Merging #1478 (e1a1c88) into main (cb09906) will increase coverage by 0.02%.
The diff coverage is 34.10%.

@@            Coverage Diff             @@
##             main    #1478      +/-   ##
==========================================
+ Coverage   51.13%   51.15%   +0.02%     
==========================================
  Files         450      450              
  Lines       15492    15492              
  Branches     3045     3045              
==========================================
+ Hits         7921     7923       +2     
+ Misses       7558     7556       -2     
  Partials       13       13              
Impacted Files Coverage Δ
packages/eas-cli/src/branch/queries.ts 22.96% <23.81%> (+0.46%) ⬆️
packages/eas-cli/src/channel/queries.ts 18.61% <28.58%> (+1.94%) ⬆️
packages/eas-cli/src/commands/update/index.ts 16.35% <60.00%> (+0.21%) ⬆️
packages/eas-cli/src/commands/channel/create.ts 32.70% <66.67%> (-0.64%) ⬇️
packages/eas-cli/src/commands/branch/create.ts 45.17% <100.00%> (+4.14%) ⬆️

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

@byCedric byCedric marked this pull request as ready for review October 25, 2022 15:13
@byCedric byCedric changed the title @bycedric/updates/refactor ensure helpers [eas-cli] Extract entity utilities from update command class Oct 25, 2022
@dsokal dsokal removed their request for review October 26, 2022 07:34
@wkozyra95 wkozyra95 removed their request for review October 26, 2022 07:57
@byCedric byCedric force-pushed the @bycedric/updates/input-refactor branch from 48eec6d to 1983d52 Compare October 28, 2022 16:03
@byCedric byCedric requested a review from EvanBacon as a code owner October 28, 2022 16:03
@byCedric byCedric force-pushed the @bycedric/updates/refactor-ensure-helpers branch from 4a7c9d6 to 40657ba Compare October 28, 2022 16:10
Base automatically changed from @bycedric/updates/input-refactor to main October 31, 2022 17:18
@byCedric byCedric force-pushed the @bycedric/updates/refactor-ensure-helpers branch from 40657ba to e1a1c88 Compare October 31, 2022 17:32
@byCedric byCedric merged commit 57f1128 into main Oct 31, 2022
@byCedric byCedric deleted the @bycedric/updates/refactor-ensure-helpers branch October 31, 2022 17:47
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