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

[Bug] Failing Additional checks since Dockerfile and docker/ubuntu/Dockerfile are different #3204

Closed
echoix opened this issue Oct 14, 2023 · 3 comments · Fixed by #3205
Closed
Labels
bug Something isn't working

Comments

@echoix
Copy link
Member

echoix commented Oct 14, 2023

Describe the bug
A clear and concise description of what the bug is.

Since #3170 modified the docker/ubuntu/Dockerfile without keeping in sync the main Dockerfile, one of the additional checks fails. It seems to have been placed exactly for this reason, to prevent diverging the two files. The PR was merged with that check indicating failure before merge.

Now every PR fails this check.

This was merged with broken Additional checks check which is actually catching an issue relevant to this PR namely enforcing intentional code duplicity.

Originally posted by @wenzeslaus in #3170 (comment)

To Reproduce
Steps to reproduce the behavior:

  1. Open any new PR based on the main branch

Expected behavior
A clear and concise description of what you expected to happen.

Keep the two files in sync.

Screenshots
If applicable, add screenshots to help explain your problem.

System description (please complete the following information):

  • Operating System: [e.g. Windows, Linux, ..., incl. version]
  • GRASS GIS version [e.g. 7.8.4]

Additional context
Add any other context about the problem here.

@neteler
Copy link
Member

neteler commented Oct 15, 2023

The "diff" test is here:

- name: Check that files with the same content are the same

How about making this a "required" test?

@echoix
Copy link
Member Author

echoix commented Oct 16, 2023

The "diff" test is here:

- name: Check that files with the same content are the same

Yep

How about making this a "required" test?

It could, but does all the additional checks need to pass? If not, the easiest might be to add that single step to the top of a required check, like the ones that build the Docker images. Since when this step could fail, it would be when changing Dockerfiles, and when we change Dockerfiles, we check that this action passes. Instead of a generic Python-related one (even if it isn't necessarily).

@echoix
Copy link
Member Author

echoix commented Oct 16, 2023

But in the meantime, this trivial PR (the changes were already longly discussed before merging the #3170) could be merged to not block any other PRs with that failing check. And that check, if ignored, prevents to see real failures that could run under it.

Edit:

Sorry I thought this was the PR, but it is the issue. See #3205 that was ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants