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

Fix helper functions for MySQL syntax #3874

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

lafriks
Copy link
Contributor

@lafriks lafriks commented Jul 5, 2024

Fixes #3870

Would be nice if someone could test it as I have no mysql/mariadb instances to reproduce this on

@lafriks lafriks added bug Something isn't working server labels Jul 5, 2024
@lafriks lafriks added this to the 2.7.0 milestone Jul 5, 2024
@rubenelshof
Copy link

I can help testing it if you want.
Please let me know if I have to do something specifically for the test.

Copy link

codecov bot commented Jul 5, 2024

Codecov Report

Attention: Patch coverage is 0% with 34 lines in your changes missing coverage. Please review.

Project coverage is 27.35%. Comparing base (fb37147) to head (d78ea31).
Report is 1 commits behind head on main.

Files Patch % Lines
server/store/datastore/migration/common.go 0.00% 34 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3874      +/-   ##
==========================================
- Coverage   27.37%   27.35%   -0.02%     
==========================================
  Files         384      384              
  Lines       26545    26572      +27     
==========================================
+ Hits         7267     7270       +3     
- Misses      18611    18636      +25     
+ Partials      667      666       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@6543 6543 added the build_pr_images If set, the CI will build images for this PR and push to Dockerhub label Jul 6, 2024
@6543
Copy link
Member

6543 commented Jul 6, 2024

I can help testing it if you want. Please let me know if I have to do something specifically for the test.

the pipeline should now publish an image you can test against

@rubenelshof
Copy link

I just tested it and it fails with the following error.

8:58AM INF woodpecker/src/github.com/woodpecker-ci/woodpecker/shared/logger/logger.go:101 > log level: debug
8:58AM FTL woodpecker/src/github.com/woodpecker-ci/woodpecker/cmd/server/main.go:46 > error running server error="can't setup store: could not migrate datastore: migration alter-table-registries-fix-required-fields failed: Error 1054 (42S22): Unknown column 'column_name' in 'where clause'"

@lafriks
Copy link
Contributor Author

lafriks commented Jul 6, 2024

as soon as CI builds new image you can try again, should be fixed now

@rubenelshof
Copy link

rubenelshof commented Jul 6, 2024

It failed again.

10:14AM INF woodpecker/src/github.com/woodpecker-ci/woodpecker/shared/logger/logger.go:101 > log level: debug
10:14AM FTL woodpecker/src/github.com/woodpecker-ci/woodpecker/cmd/server/main.go:46 > error running server error="can't setup store: could not migrate datastore: migration alter-table-registries-fix-required-fields failed: column repo_id data type in table registries can not be detected"

I dont know if this helps but I executed the query prior to the error I see in the common.go file and get the following results:

MariaDB [woodpecker]> show columns from registries;
+----------+---------------+------+-----+---------+----------------+
| Field    | Type          | Null | Key | Default | Extra          |
+----------+---------------+------+-----+---------+----------------+
| id       | bigint(20)    | NO   | PRI | NULL    | auto_increment |
| repo_id  | bigint(20)    | YES  | MUL | 0       |                |
| address  | varchar(255)  | YES  | MUL | NULL    |                |
| username | varchar(2000) | YES  |     | NULL    |                |
| password | text          | YES  |     | NULL    |                |
+----------+---------------+------+-----+---------+----------------+
5 rows in set (0.002 sec)

MariaDB [woodpecker]> show columns from registries where lower(field) = 'repo_id';
+---------+------------+------+-----+---------+-------+
| Field   | Type       | Null | Key | Default | Extra |
+---------+------------+------+-----+---------+-------+
| repo_id | bigint(20) | YES  | MUL | 0       |       |
+---------+------------+------+-----+---------+-------+

@lafriks
Copy link
Contributor Author

lafriks commented Jul 7, 2024

could be resultset is actually not lowercase as I thought, will try to use Type instead of type

@lafriks
Copy link
Contributor Author

lafriks commented Jul 7, 2024

please test again when new version is built

@rubenelshof
Copy link

This works and fixes the issue.
No errors reported in the logs.

@lafriks
Copy link
Contributor Author

lafriks commented Jul 7, 2024

can you show output of show columns from registries; after migrations?

@lafriks lafriks removed the build_pr_images If set, the CI will build images for this PR and push to Dockerhub label Jul 7, 2024
@rubenelshof
Copy link

can you show output of show columns from registries; after migrations?

+----------+---------------+------+-----+---------+----------------+
| Field    | Type          | Null | Key | Default | Extra          |
+----------+---------------+------+-----+---------+----------------+
| id       | bigint(20)    | NO   | PRI | NULL    | auto_increment |
| repo_id  | bigint(20)    | NO   | MUL | 0       |                |
| address  | varchar(255)  | NO   | MUL | NULL    |                |
| username | varchar(2000) | YES  |     | NULL    |                |
| password | text          | YES  |     | NULL    |                |
| org_id   | bigint(20)    | NO   | MUL | 0       |                |
+----------+---------------+------+-----+---------+----------------+

@lafriks
Copy link
Contributor Author

lafriks commented Jul 7, 2024

Great! Everything looks as it should be. Thanks for your help on testing it! ❤️

@lafriks lafriks requested a review from anbraten July 7, 2024 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working server
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migration fails in latest version.
3 participants