grist-core icon indicating copy to clipboard operation
grist-core copied to clipboard

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

Open hexaltation opened this issue 1 year ago • 8 comments

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

hexaltation avatar Aug 21 '24 08:08 hexaltation

Isn't it enough to just disable the workflow?

jordigh avatar Aug 22 '24 01:08 jordigh

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.

hexaltation avatar Aug 22 '24 09:08 hexaltation

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

jordigh avatar Aug 22 '24 15:08 jordigh

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

hexaltation avatar Aug 26 '24 08:08 hexaltation

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 ?

paulfitz avatar Aug 26 '24 09:08 paulfitz

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 avatar Aug 26 '24 15:08 jordigh

@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.

jordigh avatar Aug 26 '24 16:08 jordigh

@jordigh Your solution looks good to me. Thank you

hexaltation avatar Aug 27 '24 07:08 hexaltation