Skip to content

Conversation

timyarkov
Copy link
Contributor

@timyarkov timyarkov commented Apr 23, 2023

Basic multiple build tool detection (essentially converting the tool part of BuildSpec to a list)

TODO before final PR:

  • Double triple check nothing was missed
  • Rebase to staging (+squash commits)
  • Update unit tests to multi-build tool
  • Do general system testing to see if everything works as expected (did very quick test and at the very least it doesn't crash when ran on https://github.com/timyarkov/GuardianEmailClient)

@oracle-contributor-agreement
Copy link

Thank you for your pull request and welcome to our community! To contribute, please sign the Oracle Contributor Agreement (OCA).
The following contributors of this PR have not signed the OCA:

To sign the OCA, please create an Oracle account and sign the OCA in Oracle's Contributor Agreement Application.

When signing the OCA, please provide your GitHub username. After signing the OCA and getting an OCA approval from Oracle, this PR will be automatically updated.

If you are an Oracle employee, please make sure that you are a member of the main Oracle GitHub organization, and your membership in this organization is public.

@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Required At least one contributor does not have an approved Oracle Contributor Agreement. label Apr 23, 2023
@timyarkov timyarkov changed the title Multi build tool detection feat: multi build tool detection Apr 23, 2023
if not build_tool or isinstance(build_tool, NoneBuildTool):
logger.info("Unable to find a valid build tool.")
build_tools = main_ctx.dynamic_data["build_spec"]["tools"]
if len(build_tools) == 0:
Copy link
Member

Choose a reason for hiding this comment

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

You can just use if build_tools in Python to check if a list is empty or not, no need to check the length.

break

if not analyze_ctx.dynamic_data["build_spec"].get("tool"):
if len(analyze_ctx.dynamic_data["build_spec"].get("tools")) == 0:
Copy link
Member

Choose a reason for hiding this comment

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

Same comment about checking lists.

# Get the build tool identified by the mcn_version_control_system_1, which we depend on.
build_tools = ctx.dynamic_data["build_spec"].get("tools")

if len(build_tools) == 0:
Copy link
Member

Choose a reason for hiding this comment

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

Same comment about lists.

return CheckResultType.FAILED
ci_services = ctx.dynamic_data["ci_services"]

# Check for each build tool if its built as code
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# Check for each build tool if its built as code
Check if "build as code" holds for each build tool.

def run_check(self, ctx: AnalyzeContext, check_result: CheckResult) -> CheckResultType:
"""Implement the check in this method.

def _tool_check(
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def _tool_check(
def _check_build_tool(

check_result: CheckResult
) -> CheckResultType | None:
"""
Run the check for a single build tool.
Copy link
Member

Choose a reason for hiding this comment

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

Can you add why we need to do that?

ctx.commit_sha,
ci_service.api_client.get_relative_path_of_workflow(os.path.basename(bash_cmd["CI_path"])),

def _build_tool_check(
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def _build_tool_check(
def _check_build_tool(

ctx: AnalyzeContext,
check_result: CheckResult,
ci_services: list[CIInfo]
) -> CheckResultType | None:
Copy link
Member

Choose a reason for hiding this comment

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

Add docstring.

@oracle-contributor-agreement oracle-contributor-agreement bot added OCA Verified All contributors have signed the Oracle Contributor Agreement. and removed OCA Required At least one contributor does not have an approved Oracle Contributor Agreement. labels May 2, 2023
@timyarkov timyarkov force-pushed the multi_build_tool branch 7 times, most recently from 0719624 to fe20e75 Compare May 21, 2023 08:55
res = self._check_build_tool(tool, ctx, ci_services, check_result)

# Divide up build tool outputs
check_result["justification"].append("\n")
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need to add a "\n" character to divide up the outputs. The content of each element of check_result["justification"] is not used for rendering the reports. So if you add "\n" here, the string "\n" will appear instead of a new line.
Furthermore, in the HTML reports each element in check_result["justification"] is rendered as a separated bullet point in the justification column.

@timyarkov timyarkov force-pushed the multi_build_tool branch 4 times, most recently from fc50afb to d87d970 Compare June 17, 2023 04:55
@oracle-contributor-agreement
Copy link

Thank you for your pull request and welcome to our community! To contribute, please sign the Oracle Contributor Agreement (OCA).
The following contributors of this PR have not signed the OCA:

To sign the OCA, please create an Oracle account and sign the OCA in Oracle's Contributor Agreement Application.

When signing the OCA, please provide your GitHub username. After signing the OCA and getting an OCA approval from Oracle, this PR will be automatically updated.

If you are an Oracle employee, please make sure that you are a member of the main Oracle GitHub organization, and your membership in this organization is public.

@oracle-contributor-agreement oracle-contributor-agreement bot added OCA Required At least one contributor does not have an approved Oracle Contributor Agreement. and removed OCA Verified All contributors have signed the Oracle Contributor Agreement. labels Jun 18, 2023
@oracle-contributor-agreement oracle-contributor-agreement bot added OCA Verified All contributors have signed the Oracle Contributor Agreement. and removed OCA Required At least one contributor does not have an approved Oracle Contributor Agreement. labels Jun 18, 2023
@@ -3,7 +3,7 @@

# Ignore Gradle project-specific cache directory
.gradle
gradle-*
.gradle-*
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.gradle-*
gradle-*

Copy link
Member

Choose a reason for hiding this comment

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

These updates can be reverted as they don't change the expected results anymore.

Same comment for micronaut-core.json and sl4j.sjon.

@timyarkov timyarkov marked this pull request as ready for review June 18, 2023 23:06
Signed-off-by: Tim Yarkov <timdyarkov@gmail.com>
@behnazh-w behnazh-w merged commit 615a96a into oracle:staging Jun 20, 2023
art1f1c3R pushed a commit that referenced this pull request Nov 29, 2024
Signed-off-by: Tim Yarkov <timdyarkov@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OCA Verified All contributors have signed the Oracle Contributor Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants