-
Notifications
You must be signed in to change notification settings - Fork 114
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
Update CLI output to match the latest changes to the catalog commands #1597
Changes from 12 commits
544ba21
2e9d98c
aeec991
e02794b
2a264ec
f3c296b
4edaa7f
4cfe1c5
e2446ea
dec192f
2502b71
e49f3eb
3dbe9d8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -82,26 +82,22 @@ In _package.json_, set `version` to `1.0.0`: | |
|
||
```json filename=package.json lineHighlight=4 | ||
{ | ||
"private": true, | ||
"name": "ab-test", | ||
"version": "1.0.0", | ||
"scripts": { | ||
"start": "nr1 nerdpack:serve", | ||
"test": "exit 0" | ||
}, | ||
"nr1": { | ||
"uuid": "2d923ba6-d231-4dd3-830f-b1923577a422" | ||
}, | ||
"dependencies": { | ||
"prop-types": "^15.6.2", | ||
"react": "^16.6.3", | ||
"react-dom": "^16.6.3" | ||
}, | ||
"browserslist": [ | ||
"last 2 versions", | ||
"not ie < 11", | ||
"not dead" | ||
] | ||
"private": true, | ||
"name": "ab-test", | ||
"version": "1.0.0", | ||
"scripts": { | ||
"start": "nr1 nerdpack:serve", | ||
"test": "exit 0" | ||
}, | ||
"nr1": { | ||
"uuid": "2d923ba6-d231-4dd3-830f-b1923577a422" | ||
}, | ||
"dependencies": { | ||
"prop-types": "^15.6.2", | ||
"react": "^16.6.3", | ||
"react-dom": "^16.6.3" | ||
}, | ||
"browserslist": ["last 2 versions", "not ie < 11", "not dead"] | ||
} | ||
``` | ||
|
||
|
@@ -209,8 +205,8 @@ Submit your catalog information: | |
|
||
```sh | ||
nr1 catalog:submit | ||
[output] Uploading screenshots... | ||
[output] {green}✔ {plain}Screenshots uploaded | ||
[output] Uploading screenshots from nru-programmability-course/publish/ab-test... | ||
[output] {success}✔{normal} Screenshots uploaded from: nru-programmability-course/publish/ab-test | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pretty sure this section should also include the output from the Similar to what we've got over here: https://github.com/newrelic/developer-website/pull/1597/files#diff-a5b87ebe2e68e66c60aff11ad881315a991250324cd72f76a96165187dc8ab49R181-R188 Course repo for reference: https://github.com/newrelic-experimental/nru-programmability-course/tree/main/describe-app/ab-test There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let me look at this again. I was under the impression that if the screenshots folder was empty, we wouldn't try to upload it, so let me make sure that is still the case, hence why I omitted that output here. The course doesn't call for adding screenshots at the nerdpack item level, so I only included output from the root-level screenshots. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok after trying this out some more on my own, it looks like we actually don't skip screenshots on empty folders, which means that it errors out when trying to submit this step. $ nr1 catalog:submit
Uploading screenshots from /path/to/demo-app...
✔ Screenshots uploaded from: /path/to/demo-app
Uploading screenshots from /path/to/demo-app/nerdlets/home...
Uploading screenshots from /path/to/demo-app/launchers/launcher...
› Error: 1 error while updating Home 1.0.0
›
› "You must include at least 1 screenshot when uploading screenshots."
› Code: UNKNOWN There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, I see. Good point. All good to leave as is if that's how the command behaves. Pretty sure we're skipping the upload on empty There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So are these Nerdpack item-level screenshots mandatory or can we delete There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @alexronquillo per this comment, they are not required and we are releasing a patch to skip uploading screenshots directories that are empty. The output that I have above will be the correct one once that patch goes live. |
||
[output] {green}✔ {plain}Updated metadata for AbTest 1.0.0 | ||
``` | ||
|
||
|
@@ -238,18 +234,26 @@ View your catalog information: | |
|
||
```sh | ||
nr1 catalog:info | ||
[output] {purple}description: {plain}Nerdpack ab-test | ||
[output] {purple}details: {plain}Display test data for our newsletter subscription A/B test | ||
[output] {purple}displayName: {plain}AbTest | ||
[output] {purple}icon.url: {plain}https://nr3.nr-ext.net/artifact-index-production/a685fec2-29fb-40b0-9f65-4178... | ||
[output] {purple}previews.0.url: {plain}https://application-catalog-production.s3.us-east-2.amazonaws.com/nerdpacks/a... | ||
[output] {purple}releaseDate: {plain}2021-03-12T15:46:09.600138Z | ||
[output] {purple}repository: {plain}https://github.com/newrelic-experimental/nru-programmability-course | ||
[output] {purple}tagline: {plain}Win @ newsletter subscriptions | ||
[output] {purple}version: {plain}1.0.0 | ||
[output] {purple}whatsNew.changes: {plain}Initial release! Includes: | ||
[output] - A variety of charts for understanding the test r... | ||
[output] {purple}whatsNew.version: {plain}1.0.0 | ||
[output] AbTest (Nerdpack): | ||
[output] {purple}description: {plain}Nerdpack ab-test | ||
[output] {purple}details: {plain}Display test data for our newsletter subscription A/B test | ||
[output] {purple}displayName: {plain}AbTest | ||
[output] {purple}icon.url: {plain}https://nr3.nr-ext.net/artifact-index-production/a685fec2-29fb-40b0-9f65-4178... | ||
[output] {purple}previews.0.url: {plain}https://application-catalog-production.s3.us-east-2.amazonaws.com/nerdpacks/a... | ||
[output] {purple}releaseDate: {plain}2021-03-12T15:46:09.600138Z | ||
[output] {purple}repository: {plain}https://github.com/newrelic-experimental/nru-programmability-course | ||
[output] {purple}tagline: {plain}Win @ newsletter subscriptions | ||
[output] {purple}version: {plain}1.0.0 | ||
[output] {purple}whatsNew.changes: {plain}Initial release! Includes: | ||
[output] - A variety of charts for understanding the test r... | ||
[output] {purple}whatsNew.version: {plain}1.0.0 | ||
[output] AbTestLauncher (Launcher): | ||
[output] {purple}description: {plain}Describe me | ||
[output] {purple}displayName: {plain}AbTestLauncher | ||
[output] {purple}icon.url: {plain}https://nr3.nr-ext.net/artifact-index-production/a685fec2-29fb-40b0-9f65-4178... | ||
[output] AbTestNerdlet (Nerdlet): | ||
[output] {purple}displayName: {plain}AbTestNerdlet | ||
[output] {purple}supportedEntityTypes.mode: NONE | ||
``` | ||
|
||
All the information from _catalog_ shows here. | ||
|
@@ -333,4 +337,5 @@ This lesson is part of a course that teaches you how to build a New Relic One ap | |
|
||
</Callout> | ||
|
||
</HideWhenEmbedded> | ||
</HideWhenEmbedded> | ||
|
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.
Similar to the
Update your Nerdpack's catalog information
guide, how about we update the "big-picture" framing up on line 25 to have plural catalog directories:To supply information to the catalog about your app, you need to create the _catalog_ directory in your Nerdpack.
->
To supply information to the catalog about your app, you need to create _catalog_ directories in your Nerdpack.
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.
cc @alexronquillo
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.
Are multiple catalog directories actually required or are they just automatically created? In another comment there was a conversation about whether empty screenshots directories are skipped or not. I think that may factor in here too.
In general, I think the change is fine, except that this guide might need to be reworked a little bit more because the procedures following the create command only deal with adding info to the root-level catalog directory.
If these Nerdpack item-level catalog directories can be deleted/skipped, then we may need to add a step to do that. Or maybe I'm overthinking this? 😅
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.
Multiple catalog directories are automatically created but not required. Totally optional if you want to add nerdpack item screenshots or not.
We will be releasing an update to the CLI that will skip uploading any screenshots directory that is empty. Currently it is a bit annoying. If you run the
nr1 create -t catalog
then try and submit, you'll get an error stating that you need at least 1 screenshot. That shouldn't be the case, so a patch is incoming.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.
Ok, I think if the document describes the final state correctly but there's effectively a bug in the code, then let's just make the change to plural and be done with it 😃