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.

Sign up for GitHub

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

Jump to bottom

Replaced property literals with Properties constants #1912

Open
wants to merge 9 commits into
base: issues/1649
Choose a base branch
from
Open

Replaced property literals with Properties constants #1912

wants to merge 9 commits into from

Conversation

liooh
Copy link

@liooh liooh commented Oct 9, 2020 •
edited by carlspring

Pull Request Description

This pull request closes #1842

Acceptance Test

  • Building the code with mvn clean install -Dintegration.tests still works.
  • Running mvn spring-boot:run in the strongbox-web-core still starts up the application correctly.
  • Building the code and running the strongbox-distribution from a zip or tar.gz still works.
  • The tests in the strongbox-web-integration-tests still run properly.

Questions

  • Does this pull request break backward compatibility?

    • Yes
    • No

  • Does this pull request require other pull requests to be merged first?

    • Yes, please see #...
    • No

  • Does this require an update of the documentation?

    • Yes, please see strongbox/strongbox-docs#{PR_NUMBER}
    • No

Code Review And Pre-Merge Checklist

  • My code follows the coding convention of this project.
  • I have performed a self-review of my own code.
  • I have commented my code in hard-to-understand areas.
  • My changes generate no new warnings.
  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing unit tests pass locally with my changes.

@liooh
Copy link
Author

liooh commented Oct 12, 2020

I didn't understand why the pipeline contains errors, can someone help me?

sbespalov
sbespalov suggested changes Oct 13, 2020
View reviewed changes
Copy link
Member

@sbespalov sbespalov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@liooh thanks for the PR. the tests seems failed because something wrong with downloadCount, can you please double check it? Also it will be good to use static imports for Properties constants.

@liooh
Copy link
Author

liooh commented Oct 14, 2020

Thanks!
I resolved the errors and modified the imports to static imports, the pipelines seems me ok right now. @sbespalov

sbespalov
sbespalov reviewed Oct 14, 2020
View reviewed changes
...istry/src/main/java/org/carlspring/strongbox/authentication/ConfigurableProviderManager.java Outdated
@@ -218,7 +219,7 @@ public void updateAuthenticationItems(AuthenticationItems items)
Map<String, Object> properties = authenticationProvidersRegistry.getAuthenticationProperties(item.getName());

properties.put("order", item.getOrder());
properties.put("enabled", Boolean.TRUE.equals(item.getEnabled()));
properties.put(Properties.ENABLED, Boolean.TRUE.equals(item.getEnabled()));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@liooh could you please double check all the Properties usage and ensure that it used only for domain model related places?

Copy link
Author

@liooh liooh Oct 16, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just follow the informed in the task

"... All the vertices and edges property literals should be replaced with corresponding constant from org.carlspring.strongbox.db.schema.Properties ..."

What are domain model related places?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the task description seems right, and here it's not related with vertices or edges. most relevant packages are org.carlspring.strongbox.gremlin.adapters and org.carlspring.strongbox.repositories. Maybe a bit other packages as well, just check what is related with DB entities.

@sbespalov
Copy link
Member

@liooh thanks for the changes, it looks like not all imports are static. Could you please check it more?
also, please don't use merge on this branch, you should use rebase instead.

@liooh
Copy link
Author

liooh commented Oct 16, 2020

@sbespalov, I rechecked imports not static remaining and followed others recommedations marked with "resolved" in the comments.

@carlspring
Copy link
Member

@liooh thanks for the changes, it looks like not all imports are static. Could you please check it more?
also, please don't use merge on this branch, you should use rebase instead.

@liooh : Could you please only use git rebase, (instead of git merge), as outlined in our article here? Thanks! :)

@liooh
Copy link
Author

liooh commented Oct 16, 2020

@liooh thanks for the changes, it looks like not all imports are static. Could you please check it more?
also, please don't use merge on this branch, you should use rebase instead.

@liooh : Could you please only use git rebase, (instead of git merge), as outlined in our article here? Thanks! :)

Ok, alright

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbespalov sbespalov sbespalov requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@liooh @sbespalov @carlspring