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

load services using ServiceLocator if allInstances is empty #5143

Merged
merged 1 commit into from Nov 1, 2023
Merged

load services using ServiceLocator if allInstances is empty #5143

merged 1 commit into from Nov 1, 2023

Conversation

yairogen
Copy link
Contributor

@yairogen yairogen commented Oct 31, 2023

Impact

  • [x ] Bug fix (non-breaking change which fixes expected existing functionality)
  • Enhancement/New feature (adds functionality without impacting existing logic)
  • Breaking change (fix or feature that would cause existing functionality to change)

Description

fixes issue #5142

@filipelautert filipelautert self-assigned this Oct 31, 2023
@filipelautert filipelautert added TypeBug SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions labels Oct 31, 2023
Copy link

sonarcloud bot commented Oct 31, 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

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@yairogen
Copy link
Contributor Author

@filipelautert what do we need to do pass CodeQL?
how can we test this in our side?
if it works - when is the next planned release?

@filipelautert
Copy link
Collaborator

For anyone wanting to check, you can download the artifacts from here -> https://github.com/liquibase/liquibase/suites/17784962704/artifacts/1019195915

Or you can declare a new repository https://maven.pkg.github.com/liquibase/liquibase and use org.liquibase:liquibase-core:5143-copy-SNAPSHOT as the dependency, as below:

<dependency>
  <groupId>org.liquibase</groupId>
  <artifactId>liquibase-core</artifactId>
  <version>5143-copy-SNAPSHOT</version>
</dependency>

(I created this copy pr to get CodeQL running and to have the maven repo) .

For this you'll need a GitHub access token; see https://docs.github.com/en/packages/working-with-a-github-packages-registry/working-with-the-gradle-registry#using-a-published-package)

@yairogen If it works fine please let me know and we can get it merged. We may have a release in ~1-2 weeks time.

@yairogen
Copy link
Contributor Author

@filipelautert code ql failed.
Are you sure snapshot is available to test via the snapshot version and repo you provided?

@filipelautert
Copy link
Collaborator

@yairogen codeql passed here - #5144 . Currently it has a token problem when running for contributed PR's, but if a member opens the pr it works fine :/

And for the package, the instructions are from here: https://github.com/liquibase/liquibase/packages/1783578?version=5143-copy-SNAPSHOT

@npreisner
Copy link

Hi @filipelautert
Fix was tested and it works. We think it can be merged as is.
We would really appreciate if the release could be ready ASAP.
As per our security team - can you please validate there are no new vulnerabilities (CVE) introduced in the next release since that was our initial reason for upgrade (i.e. our current 3/x version has quite a few CVE issues).

regards,
Natan

@filipelautert filipelautert merged commit fa9c488 into liquibase:master Nov 1, 2023
41 of 53 checks passed
@filipelautert filipelautert added this to the 1NEXT milestone Nov 1, 2023
@yairogen
Copy link
Contributor Author

yairogen commented Nov 1, 2023

@filipelautert thanks for the merge. when can we expect official release?

filipelautert pushed a commit that referenced this pull request Nov 1, 2023
wwillard7800 added a commit that referenced this pull request Nov 1, 2023
suryaaki2 added a commit that referenced this pull request Nov 6, 2023
* WIP

DAT-16079

* Implementation of a ChangeSetService which can be used to modify
the model objects

DAT-16079

* Reformat for readability

DAT-16079

* Address CodeQL

DAT-16079

* WIP

DAT-16166

* Use the ChangeSetService for execute-sql

DAT-16166

* Change SQL parser to create change set through service

DAT-16166

* Revert "load services using ServiceLocator if allInstances is empty (#5143)"

This reverts commit fa9c488.

* Added JavaDoc

DAT-16166

---------

Co-authored-by: suryaaki2 <80348493+suryaaki2@users.noreply.github.com>
suryaaki2 added a commit that referenced this pull request Nov 16, 2023
* Revert "load services using ServiceLocator if allInstances is empty (#5143)"

This reverts commit fa9c488.

* Only reload allInstances from service locator if it's null or if we are running under OSGI

* WIP

DAT-15640

* Back out AbstractPluginFactory change

DAT-15640

---------

Co-authored-by: filipe <flautert@liquibase.com>
Co-authored-by: suryaaki2 <80348493+suryaaki2@users.noreply.github.com>
suryaaki2 pushed a commit that referenced this pull request Dec 12, 2023
* Revert "load services using ServiceLocator if allInstances is empty (#5143)"

This reverts commit fa9c488.

* Only reload allInstances from service locator if it's null or if we are running under OSGI

* Adding exceptionDetails object to structured logging

DAT-16064

* Remove change in AbstractPluginFactory that should not be there

DAT-16064

* Also show exceptionDetails on console

DAT-16064

* Also send output to console, not just log

DAT-16064

* Added another field to ExceptionDetails for exception

DAT-16064

* Handle exception when trying to get database name

DAT-16064

---------

Co-authored-by: filipe <flautert@liquibase.com>
Co-authored-by: rberezen <ruslan.berezenskyi@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
SafeToBuild Indicates that a particular PR contains changes which are safe to build using GitHub actions TypeBug
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants