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

Remove Jenkinsfile and docker-compose setup [NP-2165] #1245

Merged
merged 3 commits into from Jun 16, 2021

Conversation

davidrapson
Copy link
Contributor

@davidrapson davidrapson commented Jun 16, 2021

This supports us switching from Jenkins to Github Actions as our primary CI tool for the design system. Removes the Jenkinsfile along with any CI related Docker code.

We may want to reintroduce a docker-compose set up in future more geared towards local development. But for now our current docker set up is largely intended for CI and is not required for our actions workflows.

This might fail unless we tell Jenkins not to build anymore, let's find out.

This supports us switching from Jenkins to Github Actions as our primary
CI tool for the design system.

Removes the Jenkinsfile along with any CI related Docker code.
Copy link
Contributor

@davidsauntson davidsauntson left a comment

Choose a reason for hiding this comment

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

💥 Huzzah!

This is everything I had on my list too 👍🏻

@davidrapson
Copy link
Contributor Author

davidrapson commented Jun 16, 2021

This does indeed stall, so will wait for @simonwgill to do his bit and then we can rerun this. Good to know just how much will go though 🗑️

image

Copy link
Contributor

@luke-hill luke-hill left a comment

Choose a reason for hiding this comment

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

Sorry just saw Jenkinsfile was completely gone here too. Yeh gotta block on keeping existing behaviour first.

@simonwgill
Copy link
Contributor

Sorry just saw Jenkinsfile was completely gone here too. Yeh gotta block on keeping existing behaviour first.

What existing behaviour are you thinking of, specifically? Plan is to remove the jenkins status check and add some/all of the github actions to the branch protection rules. Anything else missing?

@luke-hill
Copy link
Contributor

Guard here: https://github.com/citizensadvice/design-system/pull/1245/files#diff-e6ffa5dc854b843b3ee3c3c28f8eae2f436c2df2b1ca299cca1fa5982e390cf8L20

browser/driver vers

There's also some other stuff in the breakout JIRA ticket, but that's not a hard blocker. I can fix up those issues tomorrow or something when I have chance

@davidrapson
Copy link
Contributor Author

davidrapson commented Jun 16, 2021

@luke-hill
Copy link
Contributor

So ideally we need the guard permissibleFail still present, as this is something which would be reduced functionality (and not desirable).
We also need to alter the browser versions (and therefore inherent driver versions), on the new yml config file, ideally mirroring the ones set in Jenkinsfile

@davidrapson
Copy link
Contributor Author

Ah gotcha! We're talking about the same stuff as on https://citizensadvice.atlassian.net/browse/NP-2166 then 😄

Will summarise what I wrote there:

  1. Happy to lock the browser versions and not use latest. I do think latest should be fine for evergreen devices, but you know the sharp-edges of BrowseStack better than I do. Will sync things up here.
  2. Nothing to reimplement for permissableFail right now as all our device combinations pass. But it's there in the form of continue-on-error using the pattern described here https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions#jobsjob_idcontinue-on-error should we need it. If the pinned browser versions result in a failure then we can use that, if not then nothing to do right now.

@luke-hill
Copy link
Contributor

So the reason for the failing one for the iPhone is it's using safaridriver 12

The short answer is this has a lot of inherent bugs in it, and can be considered flaky.

The long answer is the way in which safaridriver works can be considered JWP compatible, not W3C compatible. Back before selenium 3.2-3.5 (ruby), the drivers were in a mixed state and some supported JWP compatible endpoints and some supported W3C endpoints, safari however was not one of these. As such safari can be considered non-W3C compliant in actuality vs it being W3C-compliant technically (Like a lot of the drivers, it's closed source proprietary software). You can see evidence of this in both the hacky configs we use AND the patches further to these hacky configs.

Safari specific config: https://github.com/citizensadvice/design-system/blob/master/testing/features/support/driver.rb#L30 (This is very bad code and unsupported by capybara, and they get angry at it even being in existence)
Safari hacky overrides: https://github.com/citizensadvice/ca_testing/blob/main/lib/ca_testing/patches/capybara.rb

@davidrapson
Copy link
Contributor Author

Interesting. At least using the pinned versions as is everything seems to pass OK

image

So it looks like we can omit any extra boilerplate right now.

Comment on lines 5 to 6
on: [push]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Switched this out to test, but we should revert this before we merge.

@davidrapson davidrapson changed the title Remove Jenkinsfile and docker-compose setup Remove Jenkinsfile and docker-compose setup [NP-2165] Jun 16, 2021
@davidrapson
Copy link
Contributor Author

The short answer is this has a lot of inherent bugs in it, and can be considered flaky.

I think I misread. Is the idea that it can fail, not that it always will? In which case is it better to omit it from the matrix if we can't rely on it? Smaller stable list feels nice. If not, I think I get what you mean 👍

@luke-hill luke-hill dismissed their stale review June 16, 2021 15:41

I'll just fix up the incorrect stuff later so I'm not holding stuff up.

@davidrapson
Copy link
Contributor Author

I'll just fix up the incorrect stuff later so I'm not holding stuff up.

Sounds perfect 👍

@davidrapson davidrapson merged commit ee5b9c1 into master Jun 16, 2021
@davidrapson davidrapson deleted the remove-jenkins-build branch June 16, 2021 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants