Review Orbs Best Practices doc and tweak as needed

chrsmith commented 4 years ago

Review the CircleCI Orbs Best Practices and tweak the Pulumi orbs as needed. https://circleci.com/docs/2.0/orbs-best-practices/#orb-best-practices-guidelines https://github.com/CircleCI-Public/Orb-Policies/blob/master/Orb%20Best%20Practices%20Guidelines.md

Note: If you are looking to participate in Hacktoberfest, feel free to submit a PR with some of the recommendations applied. And feel free to test out any larger work items into a new, separate issue.

kenfdev commented 4 years ago

Reading the docs, here are some updates I think that could be made. I guess separate issues can be created for the changes that make sense 👍 WDYT?

Metadata

[ ] Define any prerequisites such as obtaining an API key in the description
- PULUMI_ACCESS_TOKEN is not explained
[ ] ~Be consistent and concise in naming your orb elements. For example, don’t mix “kebab case” and “snake case.”~ -> 2.0(since it is a breaking change)
- working_directory is snake case and skip-preview is kebab case. This shouldn't be mixed but changing this will be a breaking change.

Examples

[ ] Show orb version as x.y (patch version may not need to be included).
- Current example includes patch version

Commands

[ ] It is a good idea to check for the existance of required parameters, environment variables or other dependancies as a part of your command.
- This sounds like all the parameters that don't have a default needs an if to check the existence?

Parameters

[ ] ~Utilize the “env_var_name” parameter type to secure API keys, webhook urls or other sensitive data.~ -> 2.0(since it is a breaking change)
- It seems like PULUMI_ACCESS_TOKEN should be using env_var_name?

Executors

[ ] At least one executor per supported OS (MacOs, Windows, Docker, VM).
- There is no executor set

chrsmith commented 4 years ago

Thanks for taking look @kenfdev! Yes, filing an issue for each of those areas would be reasonable.

As far as addressing them, a few notes:

PULUMI_ACCESS_TOKEN is not explained

Not sure how familiar you are with Pulumi, but this link should probably give you all the info you need: https://www.pulumi.com/docs/intro/console/accounts-and-organizations/accounts/#access-tokens

Be consistent and concise in naming your orb elements.

We certainly should be consistent in our names and following published best practices for new orbs, but you are right that we probably shouldn't break anything right now.

What is "more standard"? I would guess we should only be using "snake_case"?

I filed https://github.com/pulumi/circleci/issues/17, an issue to track the future "Pulumi Orbs v2.0" release so we can list all of the places where we don't follow best practices and would like to make a breaking change in the future. So that way when we are ready to ship a new version, we can document what we want to update.

It seems like PULUMI_ACCESS_TOKEN should be using env_var_name?

If that is the case, then definitely. The Pulumi orbs were created before the initial release of CircleCI orbs, so it's possible env_var_name wasn't around yet. (Or equally likely, I just wasn't aware that's what we should be using.)

At least one executor per supported OS

The pulumi tool works and is tested on Linux, MacOS, and Windows. So everything should "just work" without any need for configuration differences between those executors... but when we add this I'll be sure to update our tests to use those executors and confirm that is the case.

kenfdev commented 4 years ago

Thanks again for the detailed response! I'm still new to pulumi but am excited to using it in place of terraform.

About the executor, do you have a recommendation for a default executor? Looks like the slack's orb is using cibuilds/base:latest.

https://github.com/CircleCI-Public/slack-orb/blob/staging/src/executors/alpine.yml

Another thing I noticed which is not written in the best practice is about dividing the yaml to separate files. This has downsides as well, but since the current yaml will easily grow in size, maybe it'll make sense to divide them as mentioned here

chrsmith commented 4 years ago

About the executor, do you have a recommendation for a default executor? Looks like the slack's orb is using cibuilds/base:latest.

TBH I don't know enough to have a strong opinion here.

It makes sense that we would use the pulumi/pulumi container on DockerHub: https://hub.docker.com/r/pulumi/pulumi

That container comes with pulumi "pre-installed", but doesn't have the sort of build environment a CircleCI developer might expect. So I would guess that using cibuilds/base:latest would make the most sense. Since it is the least opinionated.

Another thing I noticed which is not written in the best practice is about dividing the yaml to separate files. This has downsides as well, but since the current yaml will easily grow in size, maybe it'll make sense to divide them as mentioned here

I saw that too a while ago, but erred on the side of keeping them all in a single file. We can split them up if it ever becomes difficult to maintain, but for now things seem to be working fine IMHO.

kenfdev commented 4 years ago

I've noticed that adding a default executor without a job doesn't make much sense. IIUC, the executor defined in the orb shines when it is used inside its job.

I think it'll make sense to add a default executor when we have a job inside this orb. Do you have any thoughts on what kind of job you'd like to add @chrsmith ?

A review_app job would definitely make sense with the ability to inject steps. I wonder if any of the other individual commands would make sense as a job.

chrsmith commented 4 years ago

adding a default executor without a job doesn't make much sense. IIUC, the executor defined in the orb shines when it is used inside its job.

Yeah, you are probably right there. So let's just skip that for the time being.

Do you have any thoughts on what kind of job you'd like to add?

I can think of plenty of things along more advanced scenarios -- such as a "review app" or the automatic setup and deployment of an existing app. (e.g. deploying a new WordPress instance by just using a single command or something.)

But for now we can just get the more basic steps working, like landing pulumi stack output, stack rm, etc. and have an example of those working together before we try to take on something more sophisticated.

pulumi / circleci