-
Notifications
You must be signed in to change notification settings - Fork 40
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
tools: Migrate from CircleCi to GitHub Actions #679
tools: Migrate from CircleCi to GitHub Actions #679
Conversation
This comment was marked as off-topic.
This comment was marked as off-topic.
ce440ab
to
2fe3e2f
Compare
It LGTM so I'm not sure why it fails. Any insight? |
Have a few hunches, but nothing certain. Will let you know when I fix it and convert to non-draft. Here are the finding @orpheuslummis :
|
683d5a0
to
575aa6a
Compare
575aa6a
to
a39b49b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but I do have one question/concern before you merge.
.PHONY: test\:changes | ||
test\:changes: | ||
env DEFRA_DETECT_DATABASE_CHANGES=true gotestsum --junitfile /tmp/defradb-dev/changes.xml -- ./... -shuffle=on -p 1 | ||
env DEFRA_DETECT_DATABASE_CHANGES=true gotestsum -- ./... -shuffle=on -p 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: Trying to run this will fail because of the issue we raised yesterday. Should be remove it for now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No this will not fail. Infact we have an entire GitHub Action that is running this and passing (Detect Change).
We had two problems yesterday:
-
SSH authentication was not being done, i.e. the git command would fail. (solved by adding private and public ssh secrets and deployment key and doing the authentication in the runner).
-
Setting this env variable to false was still running the detection tests (solved by not setting this env to false explicitly anymore).
Moreover, we do have an other off case (issue #637) where with race this always fails (but rarely should this fail without race).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah dam... Missed the SSH auth part in the PR description. Sorry about that. If the keys are there then all good 🙂
a39b49b
to
702ff82
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Since we are in "code freeze" and this seems to be not |
702ff82
to
9e52dc8
Compare
- Resolves sourcenetwork#570 - Description: * Moves all our CircleCi workflows to GitHub Actions (Change Detection, Test Runs). * Change detection will only run in [PRs] or [pushes to tags, master, or develop]. * Change detection for GitHub Action Workflow needed explicit private and public ssh keys (generated and added the corresponding keys to secret section, and deploy key sections), there is an authentication step that takes care of the SSH authentication for that workflow. * Added a new Workflow to build defradb binary and call `defradb start` on it to ensure the start up command doesn't crash. * Removed CircleCi config file validation tool.
Relevant issue(s)
Resolves #570
Description
defradb start
on it to ensure the start up command doesn't crash.Merge Process
In order for a clean merge, once we decide we are ready to merge this will need to disable CircleCi completely so the failing CI check doesn't show up. In that amount of time would strongly suggest no other merges happen (since we are in code freeze anyways it should be less of an issue).
Pre-Merge Steps:
Post-Merge Steps:
develop
, maybe formaster
too (in branch protection rules).Limitations
Through this change found that CircleCi was actually not properly setting these environment variables:
So for now the CI will run without them and have made these issues:
DEFRA_BADGER_MEMORY
andDEFRA_BADGER_FILE
#682Tasks
How has this been tested?
Specify the platform(s) on which this was tested: