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 related to eliminating the dcc Dependencies and unsed tests from the code #402

Merged
merged 13 commits into from
Jun 5, 2024

Conversation

Azher2Ali
Copy link
Contributor

@Azher2Ali Azher2Ali commented Jun 3, 2024

In the current PR, we have done refactoring to the code in order to eliminate the icgc.dependency imports. These imports are being replaced with imports from an alternative library, bio.overture.score.core

The reason for this change is that there are no more published versions of the dcc packages available. This means that the code can no longer rely on the org.icgc.dcc

The refactored code imports the function from the bio.overture.score class instead. This ensures that the code will continue to function correctly.

Also, This commit removes several test dependencies related to the org.icgc.dcc libraries:

dcc-auth-server
dcc-common-core
dcc-metadata-core
dcc-metadata-client
dcc-metadata-server

These dependencies were previously included for testing purposes.

Reasoning:

  • Since the dcc dependency has also been removed , these test dependencies are no longer necessary.
  • Keeping unused dependencies can clutter the project's pom.xml file and potentially lead to issues during compilation or testing.

Moreover, we have Removed incomplete modern auth rule tests

This commit removes the following test files from the score-test module:

  • AbstractStorageIntegrationTest.java
  • PartitionedBucketFallbackIntegrationTest.java
  • PartitionedBucketIntegrationTest.java
  • StorageIntegrationTest.java

As, these tests were identified as incomplete for verifying the functionality of modern authentication rules. They likely did not cover all the necessary scenarios to ensure proper access control based on these rules. Also we have migrated the utils classes which were in libraries and used in the project

Copy link
Contributor

@joneubank joneubank left a comment

Choose a reason for hiding this comment

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

So in general this looks like we expected, removing some test cases that are no longer relevant to Score in the PartitionBucket tests, and removing DCC dependencies where they are found.

You added the dcc common utils into score server, and i think it may be more useful if those live in the score-core package so that they can be shared across the different score packages.

There is quite a bit of code commented out here that doesn't seem related to the dcc-dependencies at all. Could you add some comments to the PR or respond to mine to explain why these are removed? All these changes should be described in the PR description so that someone reading this request in the future can understand why these pieces of functionality were removed.

Additionally, heres some miscellaneous notes on handling copyright notices on the code files and thoughts on commented code:

  • Copyright notices
    • All new code files need the notice pasted in (you've added in some places, not all)
    • Update the year in the notice to the current year for every new notice you add
    • Don't need to update the year in all copyright notices, but you may choose to do this in files you are modifying already in the PR
    • If you want to update every file's copyright notice, we should do that on a separate dedicated PR to reduce noise in this PR
  • Commented Code
    • Done for the draft so its understandable that this will be removed eventually, but, theres no reason to comment it out only to remove it later. The git diff shows us the original code that was removed, so we can just remove it.

@Azher2Ali Azher2Ali changed the title Fix/dcc dependencies Fix/dccdependencies Jun 4, 2024
@Azher2Ali Azher2Ali changed the title Fix/dccdependencies Fix related to eliminating dcc Dependencies from the code Jun 4, 2024
@Azher2Ali Azher2Ali changed the title Fix related to eliminating dcc Dependencies from the code Fix related to eliminating the dcc Dependencies from the code Jun 4, 2024
@Azher2Ali Azher2Ali changed the title Fix related to eliminating the dcc Dependencies from the code Fix related to eliminating the dcc Dependencies and unsed tests from the code Jun 4, 2024
Copy link
Contributor

@joneubank joneubank left a comment

Choose a reason for hiding this comment

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

Great stuff. Only change needed is to completely remove references to the dcc artifactory.

pom.xml Outdated Show resolved Hide resolved
pom.xml Show resolved Hide resolved
score-client/pom.xml Show resolved Hide resolved
@Azher2Ali Azher2Ali requested a review from joneubank June 5, 2024 13:48
Copy link
Contributor

@joneubank joneubank left a comment

Choose a reason for hiding this comment

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

All good now! Many thanks!

@Azher2Ali Azher2Ali merged commit e264418 into develop Jun 5, 2024
2 checks passed
@Azher2Ali Azher2Ali deleted the fix/dccDependencies branch June 5, 2024 15:20
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.

2 participants