CircleCI-Public / orb-tools-orb

Various tools for authoring and publishing CircleCI orbs
https://circleci.com/orbs/registry/orb/circleci/orb-tools
MIT License
51 stars 74 forks source link

feat(jobs): add `resource_class` overrides #152

Closed wyardley closed 1 year ago

wyardley commented 2 years ago

See also CircleCI-Public/circleci-cli-orb#17

wyardley commented 2 years ago

@KyleTryon you mind taking a look at this one? Happy to make any adjustments necessary

I used resource-class instead of resource_class for consistency with this orb's overall style of using kebab vs snake case for parameters.

emmeowzing commented 2 years ago

+1 on setting the resource_class, have thought it's strange that large classes are used for these jobs when it's probably not necessary to allocate so many resources.

KyleTryon commented 2 years ago

What do yall think about using resource_class as opposed to the suggested resource-class, to keep consistent with existing config features?

@wyardley I think setting the enum now is fine until we hear requests for getting this working on runner. This wouldn't regress anything and I think the desire to use runner here should be low so I agree this is better.

Thank you for the contribution as always

wyardley commented 2 years ago

I can update it Monday to that way if you want. I can see it both ways, but kind of seemed weird to use snake case when all the other orb parameters use kebab?

wyardley commented 2 years ago

@KyleTryon another option (which I've done before for an internal orb) would be to use a yaml anchor to duplicate it with both names.

For now, I updated it to use snake case like the parameter it's a passthrough to.

wyardley commented 2 years ago

ps - also note: https://github.com/CircleCI-Public/circleci-cli-orb/pull/17 (mentioned in the description) -- if this is implemented, I could potentially update the changes here slightly differently, for example, defaulting the resource class to small there and / or supporting an additional passthrough for the resource_class that goes all the way through.

wyardley commented 2 years ago

Hi @KyleTryon - does this look like what you were expecting?

wyardley commented 2 years ago

@KyleTryon hi again - mind taking another look?

KyleTryon commented 2 years ago

Hey @wyardley sorry for the delay. Running your tests now. One request:

could we change the wording of Support setting / overriding to Configure the

So it might read such as:

resource_class:
    description: Configure the executor resource class

Also just a question, what is the motivation to changing the executor for the pack and publish jobs to be parametrizable? The job should work with any executor in theory that has the CircleCI CLI, and I believe JQ, but these jobs were designed very much in mind to run with the defined cli/default executor.


Also just wanted to let you know that I am currently working on the Orb Tools Orb this week, so I expect we will have an update out with any changes we can get in this week either by end of this week or early next.

orb-publisher commented 2 years ago

Your development orb has been published. It will expire in 30 days. You can preview what this will look like on the CircleCI Orb Registry at the following link: https://circleci.com/developer/orbs/orb/circleci/orb-tools?version=dev:5ee58876497851e6a4376c19f4a1f6d05706829e

wyardley commented 2 years ago

could we change the wording of Support setting / overriding to Configure the

updated

Also just a question, what is the motivation to changing the executor for the pack and publish jobs to be parametrizable? The job should work with any executor in theory that has the CircleCI CLI, and I believe JQ, but these jobs were designed very much in mind to run with the defined cli/default executor.

I guess I saw it as being generically useful, potentially, but the main impetus for me was to support using the same executor, but with the resource_class similarly set to small -- even if the cli orb supported a similar executor class override, there'd be no way to use it without redefining it with a different resource class, or supporting a passthrough to the passthrough. Also, the upstream orb doesn't even do anything special to the executor - it's not pinning the version so it's just using circleci/circleci-cli:latest anyway. Another option would be to use that directly in here vs using the cli orb's executor, though I understand why you all have done it this way.

So partially depends on the outcome of:

https://github.com/CircleCI-Public/circleci-cli-orb/pull/17

For example, this override ability would be less important if that executor also defaulted to small. If you are good with using a different default class in the CLI orb, and / or want to support a passthrough parameter, we could do it slightly differently if you prefer, but executor override seemed the most obvious.

So, something like:

orbs:
  orb-tools: circleci/orb-tools@11
  #cli: circleci/circleci-cli@N

executors:
  smallcli:
    resource_class: small
    docker:
      - image: circleci/circleci-cli:latest
jobs:
  orb-tools/pack:
    executor: smallcli

I don't think (because I was trying to do something like this recently) it's supported to do something like the pseudo-code below, but even if you could, you'd still need to be able to override the executor

executors:
  smallcli:
    executor:
      name: cli/default
      ## or something like this, to create a new executor inherited from cli/default and then use it later
      resource_class: small

Of course in practical terms, are these orbs using large costing us very much, especially with the frequency / duration that they run? But, since the change to the way defaults work, it's still somehow irritating to me that it's so hard to have these jobs default to large resource class.

KyleTryon commented 2 years ago

it's not pinning the version so it's just using circleci/circleci-cli:latest anyway. Another option would be to use that directly in here vs using the cli orb's executor, though I understand why you all have done it this way.

This is a fair point, the pattern is a bit of a hold-over that is no longer really needed or relevant. Moving this direction makes plenty of sense to me. Interested to maybe hear what others think.

Of course in practical terms, are these orbs using large costing us very much, especially with the frequency / duration that they run? But, since the change to the way defaults work, it's still somehow irritating to me that it's so hard to have these jobs default to large resource class.

Not that it needs this either but it should be defaulting to medium, but for orb tasks, yes small should be just fine.

wyardley commented 2 years ago

Not that it needs this either but it should be defaulting to medium, but for orb tasks, yes small should be just fine.

@KyleTryon That's the behavior I've seen with open source Circle; my understanding is that within paid plans, CircleCI chooses a default executor type for your organization that is not configurable by you. In our case, the default seems to be large. I can't find the reference for this right now, but that's the behavior I'm seeing.

Recent build with the default settings in a project within my org (and yes, this does take 7s to run, so am I a little silly for nitpicking over it, yes, but it also does feel like a bit of a money-grab that Circle chooses a default for us for all unspecified executors that's not configurable):

image
KyleTryon commented 2 years ago

Two requests.

  1. Could we enable the parameters to change the resource class, but can we keep the default at medium? This should prevent a breaking change. a. In a future major change we could set the default to small.
  2. I am realizing some shortcoming in specifying executors that I will take note of. Based on what you mentioned earlier as well, I think we should consider remove the cli orb, and reference the image directly, so we can more easily control the resource class.

What do you think?

CircleCI chooses a default executor type for your organization that is not configurable by you

This is not something I am familiar with and must be very new if so. Would you mind contact CircleCI support? I am nearly certain we would never specify a large resource class for you by default. The resource class should always default to medium unless otherwise specified, though it may be specified by the orb. One of the rare exception is we manually specify large in the android orb due to the memory needed for android builds.

Though, the default is not configurable, that is true, the default is essentially hard-coded to medium.

wyardley commented 2 years ago

This is not something I am familiar with and must be very new if so. Would you mind contact CircleCI support? I am nearly certain we would never specify a large resource class for you by default.

Hi @KyleTryon - I wrote them in Oct of 2021 (ticket #98231 if you're able to see internal tickets) JC wrote at the time:

Unfortunately, there isn't a way to set this for a specific project without changing the resource_class in your config.yml. All new project executors by default will be set to Large.

https://circleci.com/docs/optimizations also mentions:

Please note, if a resource_class is not explicitly declared, CircleCI will try to find the best default resource class for your organization.

wyardley commented 2 years ago

Ok @KyleTryon - look out for any typos / weird stuff, but let me know how this approach looks. If you want, I can take out the added executor.

I also updated the PR title / commit message, but let me know if anything should be expanded on or clarified there.

wyardley commented 2 years ago

Hi @KyleTryon How does this look now?

wyardley commented 1 year ago

Hi @KyleTryon - have you had a chance to review this?

orb-publisher commented 1 year ago

Your development orb has been published. It will expire in 30 days. You can preview what this will look like on the CircleCI Orb Registry at the following link: https://circleci.com/developer/orbs/orb/circleci/orb-tools?version=dev:7872001ee28c579345f28c29cad979181a42ae91

KyleTryon commented 1 year ago

Very sorry for the wait @wyardley and thank you again for all your effort and patience. We are finally ready! I am merging this and taking a look at one more community PR before we release the next version.

orb-publisher commented 1 year ago

Your development orb has been published. It will expire in 30 days. You can preview what this will look like on the CircleCI Orb Registry at the following link: https://circleci.com/developer/orbs/orb/circleci/orb-tools?version=dev:c34bc9dd0f8ae29e9e44f398dd07440cc979cb66

wyardley commented 1 year ago

Thanks @KyleTryon!