Skip to content

Update environment of cron service#13111

Merged
mekarpeles merged 3 commits into
masterfrom
cron-env-update
Jul 4, 2026
Merged

Update environment of cron service#13111
mekarpeles merged 3 commits into
masterfrom
cron-env-update

Conversation

@jimchamp

@jimchamp jimchamp commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator

Closes #13108

Attempts to read the config YAML in the cron job container were resulting in PermissionErrors. Adding the OL_CONFIG environment variable to the cron service, then forwarding said variable to cron's environment appears to have solved the issue.

Note that the ol-cron-start.sh changes have not been tested, per se -- I ran echo "OL_CONFIG=/olsystem/etc/openlibrary.yml" >> /etc/environment inside of the container in order to test these changes.

Technical

Testing

Screenshot

Stakeholders

@RayBB

RayBB commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator

This is a good solution.
The other thing we could do is have the system just use some default for features if OL_CONFIG isn't set but that probably leaves us in a more fragile state.

@jimchamp jimchamp added the Patch Deployed This PR has been deployed to production independently, outside of the regular deploy cycle. label Jul 2, 2026
@jimchamp

jimchamp commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator Author

This will probably be fine -- I'm not sure what a sensible default for features would be.

Will know if this is working in ~10 minutes. If it works, I'll move this out of draft.

@jimchamp

jimchamp commented Jul 3, 2026

Copy link
Copy Markdown
Collaborator Author

It did not work. Have verified that OL_CONFIG is set in the container by running echo $OL_CONFIG inside of the container.

@RayBB

RayBB commented Jul 3, 2026

Copy link
Copy Markdown
Collaborator

Does it still have the same error? Do you know if the conf yml is in the container?

@jimchamp

jimchamp commented Jul 3, 2026

Copy link
Copy Markdown
Collaborator Author

I believe that OL_CONFIG is not being exported at cron startup, and conf/openlibrary.yml is being used as a default. Attempting to verify now.

@jimchamp jimchamp marked this pull request as ready for review July 3, 2026 01:54
Copilot AI review requested due to automatic review settings July 3, 2026 01:54

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Updates the production cron-jobs container environment so cron-invoked scripts can read the intended Open Library config YAML (via OL_CONFIG), addressing PermissionError failures seen when scripts default to conf/openlibrary.yml (Issue #13108).

Changes:

  • Add OL_CONFIG=/olsystem/etc/openlibrary.yml to the cron-jobs service in compose.production.yaml.
  • Update docker/ol-cron-start.sh to expose OL_CONFIG to cron by writing it into /etc/environment before starting the daemon.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
docker/ol-cron-start.sh Forwards OL_CONFIG into cron’s environment via /etc/environment before installing the crontab and starting cron.
compose.production.yaml Sets OL_CONFIG for the cron-jobs service to point at the production config YAML in /olsystem.

Comment thread docker/ol-cron-start.sh Outdated
@jimchamp

jimchamp commented Jul 3, 2026

Copy link
Copy Markdown
Collaborator Author

Okay, I'm fairly certain that this will fix things now that the variable is being forwarded to cron's environment. We probably won't know for sure until after our next deployment, though.

For now, our cron environment has been updated, and the store counts job is working again.

@RayBB

RayBB commented Jul 3, 2026

Copy link
Copy Markdown
Collaborator

Thanks for looking into this one. Sorry about the inconvenience.

jimchamp and others added 3 commits July 3, 2026 23:50
@mekarpeles mekarpeles merged commit 980724e into master Jul 4, 2026
8 checks passed
@mekarpeles mekarpeles deleted the cron-env-update branch July 4, 2026 06:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Needs: Testing Patch Deployed This PR has been deployed to production independently, outside of the regular deploy cycle.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Recent feature flag changes have broken store_counts.py script

4 participants