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

Update CLI output to match the latest changes to the catalog commands #1597

Merged
merged 13 commits into from
Sep 10, 2021

Conversation

jerelmiller
Copy link
Contributor

Description

This PR updates the mock CLI output on multiple guides to reflect the latest changes made to the output in the nr1 CLI commands.

Reviewer Notes

NOTE: This PR will be left in draft state until we can confirm the changes have been released publicly.

@gatsby-cloud
Copy link

gatsby-cloud bot commented Sep 8, 2021

Gatsby Cloud Build Report

developer-website-develop

🎉 Your build was successful! See the Deploy preview here.

Build Details

View the build logs here.

🕐 Build time: 3m

Performance

Lighthouse report

Metric Score
Performance 🔶 33
Accessibility 🔶 88
Best Practices 🔶 87
SEO 🔶 76

🔗 View full report

Copy link
Contributor

@alexronquillo alexronquillo left a comment

Choose a reason for hiding this comment

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

@jerelmiller Thanks for this! It looks great. I just added a few small thoughts/questions.

@@ -222,15 +222,15 @@ nr1 create

<Step>

Select **catalog**, which creates a stub in your project under the `catalog` directory. Here's how the results might look in your code editor:
Select **catalog**, which creates a stub in your project under the `catalog` directory in the root directory and any launchers, nerdlets, or visualizations directories. Here's how the results might look in your code editor:
Copy link
Contributor

Choose a reason for hiding this comment

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

Here's how the results might look in your code editor:

Do you think it'd be valuable to update the following screenshot?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ya thats probably a good idea. Let me see what I can whip up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 0c7017d

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Select **catalog**, which creates a stub in your project under the `catalog` directory in the root directory and any launchers, nerdlets, or visualizations directories. Here's how the results might look in your code editor:
Select **catalog**, which creates a stub in your project under the `catalog` directory in the root directory and any launcher, Nerdlet, or visualization directories. Here's how the results might look in your code editor:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tyreer I can see how this reads better in English, but I'm wondering if wrapping it as inline code would make sense. The reason I went with plural here was because the directories are called nerdlets, launchers and visualizations. I guess the question here is whether we want to use the directory names or update to your suggestion. Thoughts? @alexronquillo do you have a preferred direction for this kind of thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh also, are nerdlets capitalized or no? I know Nerdpacks are, but wasn't sure if that also applied to Nerdlet.

Copy link
Contributor

@tyreer tyreer Sep 8, 2021

Choose a reason for hiding this comment

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

Happy to follow Alex's lead here :)

Yeah, Nerdlet in a sentence should be capitalized. Checked here: https://newrelic.slack.com/archives/G01GR8SHEHW/p1626718298004000?thread_ts=1626717697.002200&cid=G01GR8SHEHW

Referring to a directory, I think something like nerdlets (with the inline code-formatted text) would make it clear if we go that route.

Copy link
Contributor

Choose a reason for hiding this comment

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

We've been trying to move away from using code for filenames because it can be confusing at times, so I think if we want to use directory names, let's use:

...under the catalog directory in the root directory and any launchers, nerdlets, or visualizations subdirectories....

I do prefer what @tyreer suggested, though, for two reasons:

  1. It reads better in English
  2. The new catalog directories actually go into the subdirectories of those, right? So not in launchers, but actually in launchers/my-laucher

And, yep, Nerd* terms are all capitalized because they're branded terms, whereas the others are common nouns.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The new catalog directories actually go into the subdirectories of those, right? So not in launchers, but actually in launchers/my-laucher

Thats correct. It would be something like launchers/my-launcher/catalog

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in 3dbe9d8

@jerelmiller jerelmiller force-pushed the jerel/screenshot-output-updates branch from 0c7017d to 4cfe1c5 Compare September 8, 2021 19:27
Copy link
Contributor

@tyreer tyreer left a comment

Choose a reason for hiding this comment

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

This is looking fantastic! Gotta love that bash widget :D

A few suggestions in there. I struck through the list here too just to keep track and highlighted the one that seems like it needs a revision to finish off.

Thanks for going through all these!

| _additionalInfo.md_ | An optional markdown file for any additional information about using your application |
| _screenshots_ | A directory that contains screenshots of your Nerdlets or visualizations. This can contain no more than 6 images. All screenshots must meet the following criteria:<br /><br /><ul><li>3:2 aspect ratio</li><li>PNG format</li><li>landscape orientation</li><li>1600 to 2400 pixels wide</li></ul> |

This command also generates a `catalog` directory for each launcher, nerdlet, and visualization in your Nerdpack. Inside you'll find a directory that allows you to add screenshots for each artifact.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
This command also generates a `catalog` directory for each launcher, nerdlet, and visualization in your Nerdpack. Inside you'll find a directory that allows you to add screenshots for each artifact.
This command also generates a `catalog` directory for each launcher, Nerdlet, and visualization in your Nerdpack. Inside you'll find a directory that allows you to add screenshots for each Nerdpack item.

I feel like we're a bit TBD regarding capitalization of launcher (AC 5 here), but I think it's generally been non-caps in dev guides and it feels right to leave it non-caps in this update.

Nerdlet we should capitalize though, and suggesting the artifact -> Nerdpack item swap here along lines of @alexronquillo 's comments

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah good catch. I forgot about this one. Thanks!

@@ -38,13 +38,13 @@ cd nru-programmability-course/describe-app/ab-test

Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

...you need to create catalog directories...

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? 😅

Copy link
Contributor Author

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.

Copy link
Contributor

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 😃

[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
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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

Copy link
Contributor

@tyreer tyreer Sep 8, 2021

Choose a reason for hiding this comment

The 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 screenshots/ in the root directory, but we never established that for the Nerdpack-item children screenshots/ directories, which feels like an oversight

Copy link
Contributor

Choose a reason for hiding this comment

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

So are these Nerdpack item-level screenshots mandatory or can we delete screenshots/ if there aren't any?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

@@ -222,15 +222,15 @@ nr1 create

<Step>

Select **catalog**, which creates a stub in your project under the `catalog` directory. Here's how the results might look in your code editor:
Select **catalog**, which creates a stub in your project under the `catalog` directory in the root directory and any launchers, nerdlets, or visualizations directories. Here's how the results might look in your code editor:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Select **catalog**, which creates a stub in your project under the `catalog` directory in the root directory and any launchers, nerdlets, or visualizations directories. Here's how the results might look in your code editor:
Select **catalog**, which creates a stub in your project under the `catalog` directory in the root directory and any launcher, Nerdlet, or visualization directories. Here's how the results might look in your code editor:


```sh
nr1 create --type catalog
```

This creates a _catalog_ directory with template files for inputting custom information about your app.
This creates a root _catalog_ directory and a _catalog_ directory for each Nerdpack item with template files for inputting custom information about your app.
Copy link
Contributor

Choose a reason for hiding this comment

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

...a catalog directory for each Nerdpack item with template files for inputting custom information...

Elsewhere, it sounded like these Nerdpack item-level catalog directories only hold screenshots and not template files. Is that right?

Copy link
Contributor

@tyreer tyreer Sep 9, 2021

Choose a reason for hiding this comment

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

Well spotted! This would be good to specify.

You're right. At present, the only utility of the Nerdpack item directories is to provide a place where screenshots can be put and uploaded via nr1 catalog:submit. The folder structure is always <nerdpackItem>/catalog/screenshots/. Because it only contains a screenshots/ folder, the catalog/ directory in a Nerdpack item is more empty that the root level catalog/ directory.

That root level directory contains the template files this line is referring to, and the Nerdpack items directories do not.

We decided to mirror the root catalog/ directory folder structure because in future we may indeed add template files to the Nerdpack item directories—room to grow.

How about something like either of the below?

[Less descriptive]

This creates a root catalog directory with template files for inputting custom information about your app, as well as a catalog directory for each Nerdpack item.

[More descriptive]

This creates a root catalog directory with template files for inputting custom information about your app and a catalog directory for each Nerdpack item where screenshots can be stored.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @tyreer! I like the more descriptive suggestion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like this a lot. Thanks!

[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
Copy link
Contributor

Choose a reason for hiding this comment

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

So are these Nerdpack item-level screenshots mandatory or can we delete screenshots/ if there aren't any?

@@ -38,13 +38,13 @@ cd nru-programmability-course/describe-app/ab-test

Copy link
Contributor

Choose a reason for hiding this comment

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

...you need to create catalog directories...

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? 😅

@tyreer
Copy link
Contributor

tyreer commented Sep 9, 2021

@jerelmiller update that we've turned on the feature flag to enable the nerdpackItems field in all environments, and nr1 catalog:info is looking good out in prod on my personal account

@jerelmiller
Copy link
Contributor Author

@alexronquillo @tyreer all suggestions should now be in place. Thanks so much for the thorough reviews!

alexronquillo
alexronquillo previously approved these changes Sep 10, 2021
@jerelmiller jerelmiller marked this pull request as ready for review September 10, 2021 22:01
@jerelmiller jerelmiller merged commit 56d8139 into develop Sep 10, 2021
@jerelmiller jerelmiller deleted the jerel/screenshot-output-updates branch September 10, 2021 22:02
@nr-opensource-bot
Copy link
Contributor

🎉 This PR is included in version 1.52.3 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants