-
Notifications
You must be signed in to change notification settings - Fork 102
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
app versions list
and app release
#3890
Conversation
Thanks for your contribution! Depending on what you are working on, you may want to request a review from a Shopify team:
|
Coverage report
Show new covered files 🐣
Show files with reduced coverage 🔻
Test suite run success1660 tests passing in 769 suites. Report generated by 🧪jest coverage report action from 2ba0f38 |
bfecb5d
to
349169e
Compare
6a55731
to
90a1d91
Compare
We detected some changes at either packages/*/src or packages/cli-kit/assets/cli-ruby/** and there are no updates in the .changeset. |
f31515a
to
cc6cee8
Compare
appVersionId: number | ||
versionId: string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't love calling these 2 things basically the same thing, but I believe that once we switch from the prototype backend to main
I will be able to revert this anyway (since it will use a numeric ID as well - see here).
packages/app/src/cli/utilities/developer-platform-client/shopify-developers-client.test.ts
Outdated
Show resolved
Hide resolved
appVersions: { | ||
nodes: result.app.versions.map((version) => { | ||
return { | ||
createdAt: '0', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have version creation date available in the prototype API, we should make sure it's there in main.
displayName: version.createdBy.name, | ||
}, | ||
versionTag: version.versionTag, | ||
status: '', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
status
is not currently implemented
packages/app/src/cli/utilities/developer-platform-client/shopify-developers-client.ts
Outdated
Show resolved
Hide resolved
} | ||
} | ||
|
||
async appVersionsDiff( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For context on the client-side implementation (vs. server-side in PartnersClient
), see here
message: '', | ||
location: '', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
message
should be added to the API. location
is a question where it should be generated (server vs. client); it refers to a web UI URL, and I don't think we even know yet what that will be.
category: '', | ||
details: [], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we'll need to work out a lot of these details when we move to the main
backend. This applies across many operations.
The new API doesn't have the ability to produce a diff yet.
cc6cee8
to
bbe3166
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks fine and everything works as expected! 👌
Just some minor suggestions.
packages/app/src/cli/utilities/developer-platform-client/shopify-developers-client.test.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Gonzalo Riestra <gonzalo.riestra@shopify.com>
WHY are these changes introduced?
Filling out more commands
WHAT is this pull request doing?
Adds ShopifyDevelopersClient support for
app versions list
andapp release
.Note that, since a version diffing query is unavailable, we have implemented a simple local diffing strategy which is essentially the same as what's going on behind the scenes anyway.
Also note that some data (e.g. created at date) has been stubbed out for now, since it is not available in the prototype API but should be available in the production API.
These screenshots should give a general picture of the situation:
app versions list
app release
(showing diffs of adding + removing)How to test your changes?
On prototype, run
app versions list
andapp release
Measuring impact
How do we know this change was effective? Please choose one:
Checklist
dev
ordeploy
have been reflected in the internal flowchart.