gristlabs / grist-core

Grist is the evolution of spreadsheets.
https://www.getgrist.com/
Apache License 2.0
6.97k stars 308 forks source link

Forks project run `Push latest Docker image` action every day #1168

Closed hexaltation closed 1 week ago

hexaltation commented 3 weeks ago

Describe the current behavior

Every day at 5:41am GMT a Push latest Docker image action triggers in my fork. It fails and notifies me.

Some computation time that I don't want/need is wasted.

Steps to reproduce

  1. Fork the project in a github user other than gristlabs
  2. Wait 24 hours
  3. Check triggered actions

Describe the expected behavior

The workflow can't be triggered automatically in forks.

Proposed solution May be we can add a boolean variable in the CI so if not present or set at False the workflow doesn't trigger.

Where have you encountered this bug?

Instance information (when self-hosting only)

Appears only in github CI/actions

jordigh commented 3 weeks ago

Isn't it enough to just disable the workflow?

hexaltation commented 3 weeks ago

Hello @jordigh,

It surely can be an option. Especially if the problem is just the notifications.

Nevertheless I think that it could be great to test if variables mandatory to succeed the workflow are here before running a build/workflow that will inevitably fail.

And maybe It can help people forking to properly setup the environment if they want the workflow to be run an push their custom image each day to dockerHub.

Maybe by adding some steps like:

- name: check_variables 
  if: '$mandatory_variable == null'
  run: |
       echo $mandatory_variable is needed to run this workflow

It's just a simple example to give the idea. A proper script checking for all the mandatory variables and generating a message listing all the missing variables will be a far better options.

jordigh commented 3 weeks ago

How does the workflow fail for you? On my fork it's currently failing because the tests are failing. image

hexaltation commented 2 weeks ago

In my fork It fails on the Prepare image but not push it yet step. But as said, I haven't set any needed env variable.

Here are the logs:

#35 ERROR: process "/bin/sh -c yarn run build:prod" did not complete successfully: exit code: 2
------
 > [builder 14/17] RUN yarn run build:prod:
0.190 yarn run v1.22.19
0.226 $ buildtools/build.sh
0.240 Using extra app directory
0.240 + tsc --build tsconfig-ext.json
24.29 app/server/mergedServerMain.ts(74,3): error TS2741: Property 'edition' is missing in type 'Promise<IGristCoreConfig>' but required in type 'IGristCoreConfig'.
24.39 error Command failed with exit code 2.
24.39 info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
------
Dockerfile:36
--------------------
  34 |     COPY stubs /grist/stubs
  35 |     COPY buildtools /grist/buildtools
  36 | >>> RUN yarn run build:prod
  37 |     
  38 |     # Prepare material for optional pyodide sandbox
--------------------
ERROR: failed to solve: process "/bin/sh -c yarn run build:prod" did not complete successfully: exit code: 2
Error: buildx failed with: ERROR: failed to solve: process "/bin/sh -c yarn run build:prod" did not complete successfully: exit code: 2
paulfitz commented 2 weeks ago

I think it would be fine for @hexaltation to add a step that gracefully ends the workflow early if a variable needed later is missing. The only downside I see is that if you are testing that things build correctly (without caring about whether the result is pushed or not) then you'll need to tweak things a little. But streamlining things for forkers seems a bit more important. What do you think @jordigh ?

jordigh commented 2 weeks ago

Sure, I've prepared a PR to disable it: #1179

I'm a little surprised with how the workflow is failing for @hexaltation as that seems unrelated to envvars being defined or not.

jordigh commented 2 weeks ago

@hexaltation If you prefer a more complete solution than my simple fix, please let me know, and I'll close my PR in favour of yours.

hexaltation commented 2 weeks ago

@jordigh Your solution looks good to me. Thank you