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

Rename the groups table to ldapgroups #326

Merged
merged 2 commits into from
Oct 2, 2023
Merged

Conversation

wolsen
Copy link
Contributor

@wolsen wolsen commented Aug 15, 2023

The groups table name is a reserved keyword in mysql and requires that the table name be escaped with backticks in order to use the groups name. Other databases such as postgres do not support the use of the backticks in the query statements. This change renames the groups table to ldapgroups in oder to avoid this problem altogether.

This does have a dependency on the backend sql plugins to alter the schema to match the new name.

Note: I have tested this with mysql, postgres and sqlite backends. Separate PRs will be opened for each of the plugin repositories to handle the migration of the table schema.

The groups table name is a reserved keyword in mysql and requires that
the table name be escaped with backticks in order to use the groups
name. Other databases such as postgres do not support the use of the
backticks in the query statements. This change renames the groups table
to ldapgroups in oder to avoid this problem altogether.

This does have a dependency on the backend sql plugins to alter the
schema to match the new name.

Signed-off-by: Billy Olsen <billy.olsen@gmail.com>
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@ghost
Copy link

ghost commented Aug 15, 2023

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

@wolsen
Copy link
Contributor Author

wolsen commented Aug 15, 2023

Note this PR has plugin changes that are in the following PRs:

@wolsen
Copy link
Contributor Author

wolsen commented Aug 24, 2023

@Fusion would love your opinion on this one. I went the route of modifying the schema, since that solves it in the most practical sense.

@Fusion
Copy link
Collaborator

Fusion commented Sep 9, 2023

It does, and it's a good point. I need to figure out how to preserve backward compatibility, otherwise I will have to save this PR until we have a truly major release.

@Fusion
Copy link
Collaborator

Fusion commented Sep 9, 2023

Spoiler: I have recently introduce a way of handling this, called LegacyVersion, but it would still be nasty surprise for users who do not update their configuration file.

@0x0034
Copy link

0x0034 commented Sep 26, 2023

Spoiler: I have recently introduce a way of handling this, called LegacyVersion, but it would still be nasty surprise for users who do not update their configuration file.

did you fix the groups is a key word in mysql8? i'm try v2.2.1 , but also can't run on mysql8

@Fusion
Copy link
Collaborator

Fusion commented Oct 1, 2023

I had not as I have been wondering, as explained above, how to avoid making this a breaking change. Im on it today.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Oct 2, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@Fusion Fusion merged commit 675b236 into glauth:dev Oct 2, 2023
6 of 7 checks passed
@Fusion Fusion mentioned this pull request Oct 2, 2023
@Fusion
Copy link
Collaborator

Fusion commented Oct 2, 2023

@wolsen Thank you for your pull request. It is now merged and is about to be released in v2.3.0.
You do not need to do anything more as I am also updating the plugins: I modified the existing migrations mechanism to support renaming tables.

Fusion added a commit that referenced this pull request Oct 2, 2023
* build: make build vars optional and passable via env

* build: use . for BUILD_FILES var to allow buildvcs flag

* build: create mkbindir, build and sha256 targets

* build: reuse build target for each platform specific compilation

* Ensure same go version for main and plugins

* chore: release 2.3.0

Release-As: 2.3.0

* release-please

* Fix(plugins)!: Rename the groups table to ldapgroups (#326)

The groups table name is a reserved keyword in mysql and requires that
the table name be escaped with backticks in order to use the groups
name. Other databases such as postgres do not support the use of the
backticks in the query statements. This change renames the groups table
to ldapgroups in oder to avoid this problem altogether.

This does have a dependency on the backend sql plugins to alter the
schema to match the new name.

Signed-off-by: Billy Olsen <billy.olsen@gmail.com>
Co-authored-by: Chris F Ravenscroft <chris@voilaweb.com>

* chore: update docker sqlite with ldapgroups (#338)

* feat: Update migration code to support table names (#339)

* chore(dev): release 2.3.0 (#337)

---------

Signed-off-by: Billy Olsen <billy.olsen@gmail.com>
Co-authored-by: shipperizer <alexcabb@gmail.com>
Co-authored-by: Billy Olsen <wolsen@users.noreply.github.com>
Fusion added a commit that referenced this pull request Oct 7, 2023
* build: make build vars optional and passable via env

* build: use . for BUILD_FILES var to allow buildvcs flag

* build: create mkbindir, build and sha256 targets

* build: reuse build target for each platform specific compilation

* Ensure same go version for main and plugins

* chore: release 2.3.0

Release-As: 2.3.0

* release-please

* Fix(plugins)!: Rename the groups table to ldapgroups (#326)

The groups table name is a reserved keyword in mysql and requires that
the table name be escaped with backticks in order to use the groups
name. Other databases such as postgres do not support the use of the
backticks in the query statements. This change renames the groups table
to ldapgroups in oder to avoid this problem altogether.

This does have a dependency on the backend sql plugins to alter the
schema to match the new name.

Signed-off-by: Billy Olsen <billy.olsen@gmail.com>
Co-authored-by: Chris F Ravenscroft <chris@voilaweb.com>

* chore: update docker sqlite with ldapgroups (#338)

* feat: Update migration code to support table names (#339)

* chore(dev): release 2.3.0 (#337)

* ci: change pr name pattern for release please

* ci: switch to use master as default branch and plan to drop dev

* fix: exploit release-please for tagging in code

* fix: use runtime/debug to fetch git build info

* fix: use newer function to fetch git info for version

* build: drop build tags

---------

Signed-off-by: Billy Olsen <billy.olsen@gmail.com>
Co-authored-by: shipperizer <alexcabb@gmail.com>
Co-authored-by: Billy Olsen <wolsen@users.noreply.github.com>
Fusion added a commit that referenced this pull request Oct 8, 2023
* build: make build vars optional and passable via env

* build: use . for BUILD_FILES var to allow buildvcs flag

* build: create mkbindir, build and sha256 targets

* build: reuse build target for each platform specific compilation

* Ensure same go version for main and plugins

* chore: release 2.3.0

Release-As: 2.3.0

* release-please

* Fix(plugins)!: Rename the groups table to ldapgroups (#326)

The groups table name is a reserved keyword in mysql and requires that
the table name be escaped with backticks in order to use the groups
name. Other databases such as postgres do not support the use of the
backticks in the query statements. This change renames the groups table
to ldapgroups in oder to avoid this problem altogether.

This does have a dependency on the backend sql plugins to alter the
schema to match the new name.

Signed-off-by: Billy Olsen <billy.olsen@gmail.com>
Co-authored-by: Chris F Ravenscroft <chris@voilaweb.com>

* chore: update docker sqlite with ldapgroups (#338)

* feat: Update migration code to support table names (#339)

* chore(dev): release 2.3.0 (#337)

* ci: change pr name pattern for release please

* ci: switch to use master as default branch and plan to drop dev

* fix: exploit release-please for tagging in code

* fix: use runtime/debug to fetch git build info

* fix: use newer function to fetch git info for version

* build: drop build tags

* feat: graceful shutdown (#343)

* fix: start servers on separate goroutines

* fix: listen to signals for graceful shutdowns

* fix: exit with 0 in case of programmed termination

---------

Signed-off-by: Billy Olsen <billy.olsen@gmail.com>
Co-authored-by: shipperizer <alexcabb@gmail.com>
Co-authored-by: Billy Olsen <wolsen@users.noreply.github.com>
@wolsen wolsen deleted the rename-groups-table branch November 29, 2023 02:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants