-
Notifications
You must be signed in to change notification settings - Fork 60
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
ALM - Project Migration #3972
ALM - Project Migration #3972
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -656,6 +656,26 @@ public static bool CreateGingerOpsConfiguration() | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return false; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
public static bool IsConfigPackageExists(string PackagePath, GingerCoreNET.ALMLib.ALMIntegrationEnums.eALMType eALMType) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
string settingsFolder = string.Empty; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
settingsFolder = eALMType switch | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GingerCoreNET.ALMLib.ALMIntegrationEnums.eALMType.Jira => "JiraSettings", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GingerCoreNET.ALMLib.ALMIntegrationEnums.eALMType.Qtest => "QTestSettings", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
_ => "JiraSettings", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+663
to
+668
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Revise the switch expression's default case. The current default case returns "JiraSettings" which could be misleading for unsupported ALM types. Consider either:
Apply this diff: settingsFolder = eALMType switch
{
GingerCoreNET.ALMLib.ALMIntegrationEnums.eALMType.Jira => "JiraSettings",
GingerCoreNET.ALMLib.ALMIntegrationEnums.eALMType.Qtest => "QTestSettings",
- _ => "JiraSettings",
+ _ => throw new ArgumentException($"Unsupported ALM type: {eALMType}", nameof(eALMType))
}; 📝 Committable suggestion
Suggested change
Comment on lines
+662
to
+668
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider improving maintainability of settings folder names. The hardcoded folder names could make maintenance difficult. Consider:
Apply this diff to improve maintainability: + private static readonly Dictionary<GingerCoreNET.ALMLib.ALMIntegrationEnums.eALMType, string> ALMSettingsFolders = new()
+ {
+ { GingerCoreNET.ALMLib.ALMIntegrationEnums.eALMType.Jira, "JiraSettings" },
+ { GingerCoreNET.ALMLib.ALMIntegrationEnums.eALMType.Qtest, "QTestSettings" }
+ };
+
public static bool IsConfigPackageExists(string PackagePath, GingerCoreNET.ALMLib.ALMIntegrationEnums.eALMType eALMType)
{
- string settingsFolder = string.Empty;
- settingsFolder = eALMType switch
- {
- GingerCoreNET.ALMLib.ALMIntegrationEnums.eALMType.Jira => "JiraSettings",
- GingerCoreNET.ALMLib.ALMIntegrationEnums.eALMType.Qtest => "QTestSettings",
- _ => "JiraSettings",
- };
+ if (!ALMSettingsFolders.TryGetValue(eALMType, out string settingsFolder))
+ {
+ throw new ArgumentException($"Unsupported ALM type: {eALMType}", nameof(eALMType));
+ } 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if (Directory.Exists(Path.Combine(PackagePath, settingsFolder))) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return true; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
else | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Reporter.ToLog(eLogLevel.WARN, "Configuration package not exist in solution, Settings not exist at: " + Path.Combine(PackagePath, settingsFolder)); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return false; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+660
to
+678
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add input validation and security checks. The method should validate the input parameters to prevent potential issues:
Consider applying this diff: public static bool IsConfigPackageExists(string PackagePath, GingerCoreNET.ALMLib.ALMIntegrationEnums.eALMType eALMType)
{
+ if (string.IsNullOrEmpty(PackagePath))
+ {
+ Reporter.ToLog(eLogLevel.WARN, "PackagePath cannot be null or empty");
+ return false;
+ }
+
+ // Sanitize path to prevent path traversal
+ PackagePath = Path.GetFullPath(PackagePath);
+ if (!PackagePath.StartsWith(WorkSpace.Instance.Solution.Folder))
+ {
+ Reporter.ToLog(eLogLevel.WARN, $"PackagePath must be within solution folder: {PackagePath}");
+ return false;
+ }
+
string settingsFolder = string.Empty;
settingsFolder = eALMType switch
{
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -290,6 +290,22 @@ private object UpdateALMType(eALMType almType) | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
case eALMType.Azure: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
almCore = new AzureDevOpsCore(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
break; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
case eALMType.Qtest: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
almCore = new QtestCore(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
break; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
case eALMType.QC: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if (CurrentAlmConfigurations.UseRest) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
almCore = new QCRestAPICore(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
else | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
almCore = new QCCore(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
break; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
case eALMType.RQM: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
almCore = new RQMCore(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
break; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+293
to
+308
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider implementing a factory pattern for ALM core instantiation. The switch statement for ALM type handling is growing and could benefit from a more maintainable design pattern. Consider extracting this logic into a factory class to improve maintainability and testability. Here's a suggested implementation: public interface IALMCoreFactory
{
ALMCore CreateALMCore(eALMType almType, ALMConfig config);
}
public class ALMCoreFactory : IALMCoreFactory
{
public ALMCore CreateALMCore(eALMType almType, ALMConfig config)
{
if (config == null)
{
Reporter.ToLog(eLogLevel.ERROR, $"Missing configuration for {almType}");
return null;
}
ALMCore core = almType switch
{
eALMType.Qtest => new QtestCore(),
eALMType.QC when config.UseRest => new QCRestAPICore(),
eALMType.QC => new QCCore(),
eALMType.RQM => new RQMCore(),
_ => null
};
if (core == null)
{
Reporter.ToLog(eLogLevel.ERROR, $"Failed to initialize {almType} core");
}
return core;
}
} Then update the -private object UpdateALMType(eALMType almType)
+private object UpdateALMType(eALMType almType, IALMCoreFactory factory = null)
{
- ALMCore almCore = null;
ALMConfig CurrentAlmConfigurations = ALMCore.GetCurrentAlmConfig(almType);
ALMCore.DefaultAlmConfig = CurrentAlmConfigurations;
- //Set ALMRepo
- switch (almType)
- {
- case eALMType.Qtest:
- almCore = new QtestCore();
- break;
- // ... other cases
- }
+
+ factory ??= new ALMCoreFactory();
+ ALMCore almCore = factory.CreateALMCore(almType, CurrentAlmConfigurations);
return almCore;
} Ensure consistent error handling across ALM implementations. The new ALM type implementations should follow the same error handling patterns as existing ones. A few observations:
Consider applying this more robust implementation: case eALMType.Qtest:
- almCore = new QtestCore();
+ if (CurrentAlmConfigurations != null)
+ {
+ almCore = new QtestCore();
+ if (almCore == null)
+ {
+ Reporter.ToLog(eLogLevel.ERROR, $"Failed to initialize QtestCore");
+ }
+ }
+ else
+ {
+ Reporter.ToLog(eLogLevel.ERROR, $"Missing Qtest configuration");
+ }
break;
case eALMType.QC:
if (CurrentAlmConfigurations.UseRest)
{
almCore = new QCRestAPICore();
+ if (almCore == null)
+ {
+ Reporter.ToLog(eLogLevel.ERROR, $"Failed to initialize QCRestAPICore");
+ }
}
else
{
almCore = new QCCore();
+ if (almCore == null)
+ {
+ Reporter.ToLog(eLogLevel.ERROR, $"Failed to initialize QCCore");
+ }
}
break;
case eALMType.RQM:
- almCore = new RQMCore();
+ if (CurrentAlmConfigurations != null)
+ {
+ almCore = new RQMCore();
+ if (almCore == null)
+ {
+ Reporter.ToLog(eLogLevel.ERROR, $"Failed to initialize RQMCore");
+ }
+ }
+ else
+ {
+ Reporter.ToLog(eLogLevel.ERROR, $"Missing RQM configuration");
+ }
break; 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
default: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Reporter.ToLog(eLogLevel.ERROR, $"Invalid ALM Type - {almType}"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
break; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
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.
Add try-finally block for cursor management
The cursor should be reset even if an exception occurs during the RQM operation. Consider wrapping the code in a try-finally block.
Here's the suggested fix:
📝 Committable suggestion