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

CircleCI Review #1

Closed
KyleTryon opened this issue Mar 22, 2021 · 2 comments
Closed

CircleCI Review #1

KyleTryon opened this issue Mar 22, 2021 · 2 comments
Labels
bug Something isn't working Status: Ready for Test

Comments

@KyleTryon
Copy link

Hello All,

My name is Kyle from the CircleCI Community and Partner Engineering team following up on my email with some review items on the current orb.

  1. If the command is not meant to be used by end-users, do not create it.
    a. Instead, simply contain all logic within the job. No commands are needed.
    b. https://github.com/k6io/circleci-orb/blob/main/src/commands/run-test.yml
  2. Allow the user to specify a specific docker tag.
    a. https://github.com/k6io/circleci-orb/blob/a1ae46a30dcd2fd80640c3a98f2fb3c95f1771fe/src/jobs/test.yml#L4
    b. Parameterize the tag portion of the docker image.
    c. Specifying a tag is important for CI so users can test against known and good versions.
    e. Example: https://github.com/CircleCI-Public/node-orb/blob/3d313ec2fbc9dd2a5c804d9b142e2419c4176ca4/src/executors/default.yml#L14
  3. Private information should be accessed via environment variables, not hard-coded strings in the config.
    a. https://github.com/k6io/circleci-orb/blob/a1ae46a30dcd2fd80640c3a98f2fb3c95f1771fe/src/jobs/test.yml#L23
    b. https://circleci.com/docs/2.0/orb-author-faq/?section=configuration#secure-api-tokens
@KyleTryon KyleTryon added the bug Something isn't working label Mar 22, 2021
@simskij
Copy link
Contributor

simskij commented Mar 23, 2021

@KyleTryon Thanks a bunch for the review! I just pushed a new version of the orb to both GitHub and the orb registry, which I think takes care of the items on your list. Would you mind having another look?

@KyleTryon
Copy link
Author

KyleTryon commented Mar 24, 2021

@simskij In the future, I recommend utilizing pull requests from another to implement changes. This way, the dev version can be updated automatically and tested prior to merging changes to main. This also gives you the opportunity to designate what type of orb update to make to the registry (if providing a production update).

I do not see your orb listed on the registry yet: https://circleci.com/orbs/registry/orb/k6io/circleci-orb
We will get this published be merging a PR to main with [semver:major] in the title.

https://circleci.com/docs/2.0/creating-orbs/#issue-a-new-release


Review items:


The first item above appears to be the only required change. Let's issue this change and publish version 1.0.0 publically to the orb registry using the method mentioned above.

Once the change is made and the orb is published we should be good to go 👍

@simskij simskij closed this as completed Apr 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Status: Ready for Test
Projects
None yet
Development

No branches or pull requests

2 participants