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

CI fixes #19

Closed
wants to merge 12 commits into from
Closed

CI fixes #19

wants to merge 12 commits into from

Conversation

ublefo
Copy link
Collaborator

@ublefo ublefo commented Mar 26, 2024

Please merge #18 first. This took me weeks to figure out and test, now everything is finally working as they should.

Improvements made:

  1. Implemented container caching to bring turnaround time from 52 minutes to 16 minutes after the container is built successfully.
  2. Set up redis in the workflow and make sure sidekiq is configured to use the correct URL from the environment if it's present.
  3. Upgrade all workflows to the latest versions.
  4. Install ruby-lsp in the development environment for the corresponding vscode extension, which is currently already installed in the devcontainer but completely useless due to the gem being missing.
  5. Implement better error reporting in database_populator.rb to report the error messages when there's something wrong with the test data. Troubleshooting test db initialization issues should be much easier now.
  6. Documented the two new redis related env variables.
  7. Skip unit tests for documentation updates.
  8. Created a dependabot config to check for github actions workflow updates weekly.

Instruction for reviewers: just check the CI workflows below and they should be all passing now.

@maddernd
Copy link

Have these changes been tested on Mac and Windows?

@maddernd maddernd requested a review from macite March 26, 2024 21:46
@satikaj
Copy link
Collaborator

satikaj commented Mar 27, 2024

Unit tests work on Mac. I tested the CI GitHub actions workflow on my fork too for these commits and they all succeed now. The implemented cache functionality also makes it run faster the second time around.

Copy link

@macite macite left a comment

Choose a reason for hiding this comment

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

This looks very good. If we can get that one new environment variable explained alongside the other environment variables in the README I think... then it should be good to merge.

config/initializers/sidekiq.rb Show resolved Hide resolved
@macite
Copy link

macite commented Mar 27, 2024

Once this is addressed you can make the PR to the main repo.

@ublefo ublefo requested a review from macite March 27, 2024 16:48
@maddernd
Copy link

This one looks good now please make upstream.

@ublefo
Copy link
Collaborator Author

ublefo commented Mar 28, 2024

Pull request opened upstream: doubtfire-lms#434

@ublefo ublefo closed this Mar 28, 2024
@ublefo ublefo deleted the ci-fix branch April 15, 2024 03:29
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.

4 participants