Skip to content
This repository was archived by the owner on Feb 26, 2021. It is now read-only.

Conversation

@fotinakis
Copy link
Contributor

No description provided.

@fotinakis fotinakis requested a review from timhaines September 27, 2018 03:13
@teeberg
Copy link
Contributor

teeberg commented Sep 27, 2018

Note that the workflow id changes when you "Rerun failed builds". I.e. if you send snapshots from multiple jobs, this will make it create a new percy build with potentially missing snapshots.

@fotinakis
Copy link
Contributor Author

@teeberg interesting point — are you sure? I'd expect "Rerun Failed Jobs" should only affect the jobs within the workflow, not the instance of the workflow itself. Admittedly I haven't tested it.

@teeberg
Copy link
Contributor

teeberg commented Sep 27, 2018

I am :) There isn't really documentation about these environment variables as far as I could find, but we ended up using CIRCLE_WORKFLOW_WORKSPACE_ID instead as the parallel nonce, since the workspace needs to stay the same between reruns. We've been using this for over a year now, so I can safely say this is pretty reliable.

@teeberg
Copy link
Contributor

teeberg commented Sep 27, 2018

As for

I'd expect "Rerun Failed Jobs" should only affect the jobs within the workflow, not the instance of the workflow itself. Admittedly I haven't tested it.

Rerunning failed jobs actually creates a new workflow. You can see that on the workflows dashboard and, when you click into the new workflow, its new URL with a new workflow ID

@fotinakis
Copy link
Contributor Author

Yup I just tried this and it seems like that's the behavior (also you're correct, the ONLY documentation I see on the web for that env var is your comments 😶 ). I'm not sure if this is 100% what we want though, because you can also "Re-run" and entire build that was successful — say if you want to re-trigger a deployment or something. In that case we would want Percy to create a new build and not try to re-use the old nonce. I'm not sure which is more common.

@teeberg
Copy link
Contributor

teeberg commented Sep 27, 2018

with "Re-run an entire build", you mean rerun the whole workflow? In that case, you get a new workspace ID, so I think that shouldn't be an issue.

Our issues with this approach are different: If you have to resort to using the workspace ID, that means you're creating the percy build from different circle jobs. As soon as you start doing that and want to support rerunning failed jobs, now you have to figure out how many parallel_total_shards to declare, since that number stops being predictable if you start retrying failed jobs. So we end up specifying 50 total shards, even though only ~12 circle containers actually create builds, and then , in a final job, keep finalizing the build until we get a 'Can only finalize pending builds' response from your API, which is far from optimal, but lets us retry failed jobs a few times if ever necessary.

If I had to re-do our percy integration today, I would write all percy artifacts to the workspace and have a final teardown job at the end of the whole test suite to submit them all. Then there would be no parallelism, no worrying about which undocumented env variable to use, and you could rerun your failed jobs as many times as you like. But I guess dictating how to use CircleCI is outside of this library's scope :)

@fotinakis fotinakis changed the title Support Circle 2.0 CIRCLE_WORKFLOW_ID as parallel nonce. Support Circle 2.0 CIRCLE_WORKFLOW_WORKSPACE_ID as parallel nonce. Oct 19, 2018
Copy link
Contributor

@djones djones left a comment

Choose a reason for hiding this comment

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

🍍 LGTM

@fotinakis
Copy link
Contributor Author

@teeberg we have tested and confirmed that CIRCLE_WORKFLOW_WORKSPACE_ID is what we want. :) Shipping this.

@fotinakis fotinakis merged commit f24246d into master Oct 19, 2018
@fotinakis fotinakis deleted the circle2.0 branch October 19, 2018 18:56
@fotinakis
Copy link
Contributor Author

Released in 2.0.1

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants