-
Notifications
You must be signed in to change notification settings - Fork 9
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
#122 Refactoring the Foundation Core Python module; removing aiops references. #163
#122 Refactoring the Foundation Core Python module; removing aiops references. #163
Conversation
habibimoiz
commented
Jun 21, 2024
- Renamed the Foundation Metadata Core (Python) package.
- Renamed the Foundation Encryption Policy (Python) package.
- Renamed the Extensions Encryption Vault Python package.
- Renamed the Extensions Data Delivery Spark (Python) package.
- Renamed all the child directories, classes, and methods.
- Corrected all the imports inside of the velocity templates to ensure a successful build of the Foundation Core Python.
- Baton migration scripts for automated migrations from legacy package names to the new aiSSEMBLE landscape.
@@ -12,6 +12,7 @@ | |||
|
|||
import com.boozallen.aiops.mda.generator.common.PipelineImplementationEnum; | |||
import com.boozallen.aiops.mda.generator.util.SemanticDataUtil; | |||
import com.boozallen.aiops.mda.metamodel.AIOpsModelInstanceRepostory; | |||
import com.boozallen.mda.maven.ArtifactType; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming aiops
is to remain for the packages in lines 13 - 15.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this a Java module. This PR is to focus on Python modules.
@@ -10,7 +10,6 @@ | |||
* #L% | |||
*/ | |||
|
|||
import com.boozallen.aiops.mda.ManualActionNotificationService; | |||
import com.boozallen.aiops.mda.generator.common.MachineLearningStrategy; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we getting rid of these imports?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing they were not being used in that particular java file (import optimization)
but I'll leave it to Moiz to verify
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that was the case. This import was removed when the foundation-mda module was built.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume the migrations are NOT final in the PR?
if(algorithm == 'VAULT_ENCRYPT'): | ||
aiops_encrypt = VaultRemoteEncryptionStrategy() | ||
aissemble_encrypt = VaultRemoteEncryptionStrategy() | ||
|
||
for record in inbound: | ||
for column in fields_to_update: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
S: this and the following CustomRecord
change look off from a quick scan...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made these changes when my build was failing due to the Pylint implementation. I wasn't aware that I needed to run git clean -dfx
in order to fix this issue. I reverted the change.
import java.util.stream.Collectors; | ||
|
||
import static com.boozallen.aissemble.upgrade.util.FileUtils.replaceLiteralInFile; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
S: we'd like all classes to have a class-level comment. I generally used anything public or not obvious as my markers to add javadoc
import java.io.BufferedReader; | ||
import java.io.File; | ||
import java.io.FileReader; | ||
import java.util.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A: no wildcard imports please - they can have unintended consequences: https://rules.sonarsource.com/java/rspec-2208/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to remove the wildcard import.
} | ||
} | ||
} catch (Exception e) { | ||
logger.error("Error in determining whether a 'AIOPS' renamed python package requires a migration."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
S: log or throw, but don't do both
*/ | ||
@Override | ||
protected boolean shouldExecuteOnFile(File file) { | ||
System.out.println("Inside should execute on file......*******"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
S: should not be using System.out.println
- use logger instead (or just remove).
If this file will be the target of the follow on ticket, recommend a preemptive comment to let the reviewer know.
import java.util.Map; | ||
|
||
import static com.boozallen.aissemble.upgrade.util.FileUtils.replaceLiteralInFile; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
S: similar comment to the prior file
try { | ||
isMigrated = FileUtils.contentEquals(original, migrated); | ||
} catch (Exception e) { | ||
e.printStackTrace(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
S: use logger
protected boolean shouldExecuteOnFile(File file) { | ||
boolean shouldExecute = false; | ||
|
||
// getMavenProject().getPlugin(""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can remove this comment
boolean isMigrated = false; | ||
|
||
try { | ||
// // I am not sure how to rename the file automatically, is this something that could potentially be a part of the manual upgrade instructions? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we remove this? or is this a comment for future reference (for the follow on ticket)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will remove this.
while((lineOne = aiopsReferencePythonPropertiesFileConfig.readLine()) !=null && !shouldExecute) { | ||
for (String key : AIOPS_REFERENCE_PYTHON_PROPERTIES_FILE_PACKAGE_MAP.keySet()) { | ||
if (lineOne.contains(key)) { | ||
return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be easier to read if instead of returning, we set shouldExecute to true and have just one return statement
97fca8c
to
235ec3d
Compare
- Renamed the Foundation Metadata Core (Python) package. - Renamed the Foundation Encryption Policy (Python) package. - Renamed the Extensions Encryption Vault Python package. - Renamed the Extensions Data Delivery Spark (Python) package. - Renamed all the child directories, classes, and methods. - Corrected all the imports inside of the velocity templates to ensure a successful build of the Foundation Core Python. - Baton migration scripts for automated migrations from legacy package names to the new aiSSEMBLE landscape.
235ec3d
to
cc04fa9
Compare
The foundation-core-python module migrations are final (AiopsReferencePythonMigration.java), however, NOT the PDP migrations. |