eddiewebb / circleci-queue

CircleCI orb to block/queue jobs to enforce max concurrency limits
MIT License
74 stars 75 forks source link

Fix environment variable issues #13

Closed zephraph closed 6 years ago

zephraph commented 6 years ago

Ran into a few issues as detailed here: https://github.com/artsy/reaction/pull/1517.

This fix will do two things:

  1. Make CIRCLECI_API_KEY not required (but warn if it isn't present)
  2. Update the oldest job logic to not fail if a job isn't detected
zephraph commented 6 years ago

So I added -f to the curl command. That'll make curl return a non-zero result if the response is not a 2xx code. The next step is to update the oldest queue code...

https://github.com/zephraph/circleci-queue/blob/env-fix/src/commands/until_front_of_line.yml#L88-L91

~This part is being triggered (in our case) when the result is [ ]. I need to dig into the test responses a bit, but I think this error handling needs to be updated.~

Actually, I just realized the above was wrong. The reason why the result was empty is because I manually ran the api later and nothing was running. I still think something is happening here, because even if the api key was invalid the result would still show up properly because it's a public repo. (It did in my results). I can confirm that we have programmed works though... so I think it's likely something else.

Here's the failing build in question:

https://circleci.com/gh/artsy/reaction/15199

zephraph commented 6 years ago

Maybe having an ignore-forks option that defaults to true would be good. Using something like CIRCLE_PR_REPONAME.

Kinda like this?

if [ ! -z "$CIRCLE_PR_REPONAME" ] && [ "<<parameters.ignore-forks>>" == "true" ]; then
   echo "Skipping queue for forked build"
else
   echo "Not a fork or fork queues allowed"
fi

I'm not sure if it makes sense to have it run on forks at all... I can't really imagine a use-case where you would want to queue things on a fork. Either way..

eddiewebb commented 6 years ago

Justin thanks for your continued investment. I like the idea, did you test how forks will work if not ignored?

I am concerned that we need to make 2 api calls if using a fork: 1) is my current (forked) project already building? 2) is parent/upstream project already building?

(line 68 in your proposed)

Or is the use case only concerned with use case 1? If so we should add some helpful language in the parameter description.

Hope that makes sense!

zephraph commented 6 years ago

I'm 👍 for just not supporting queuing on a fork. I think it's a bit of a corner case that'd cause more headache than it's worth, especially if we have to make extra api calls. I'll update the PR accordingly.