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

Only use buildRef if it exists #42

Merged
merged 1 commit into from
Nov 25, 2024

Conversation

aayushshah15
Copy link

@aayushshah15 aayushshah15 commented Nov 25, 2024

Important

Ensure reportBuildCompleted() in main.ts handles missing stateHelper.buildRef by using the first available summary.

  • Behavior:
    • In reportBuildCompleted() in main.ts, check if stateHelper.buildRef exists before using it.
    • If stateHelper.buildRef is not found in exportRes.summaries, use the first summary available.
    • Only calculate cachedRatio and set requestOptions if buildRefSummary is defined.
  • Logging:
    • Remove debug and warning logs related to exportRes and stateHelper.buildRef.

This description was created by Ellipsis for 22dd9b8. It will automatically update as commits are pushed.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to b3a756a in 15 seconds

More details
  • Looked at 36 lines of code in 1 files
  • Skipped 2 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. src/main.ts:68
  • Draft comment:
    Using optional chaining with stateHelper?.buildRef is unnecessary since stateHelper is a module and should always be defined. Consider using stateHelper.buildRef instead. This applies to line 68 and other similar usages.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The use of optional chaining with stateHelper?.buildRef is unnecessary since stateHelper is imported as a module and should always be defined. This pattern is used in multiple places in the code.

Workflow ID: wflow_k3D658nU9jr4Xuuq


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

3 days left in your free trial, upgrade for $20/seat/month or contact us.

@aayushshah15 aayushshah15 force-pushed the 11-25-Only_use_buildRef_if_it_exists branch from b3a756a to b800e6a Compare November 25, 2024 19:49
Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

❌ Changes requested. Incremental review on b800e6a in 2 minutes and 56 seconds

More details
  • Looked at 38 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 0 drafted comments based on config settings.

Workflow ID: wflow_id2c3g5fhIv8w4v4


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

3 days left in your free trial, upgrade for $20/seat/month or contact us.

src/main.ts Outdated
@@ -64,15 +64,26 @@ async function reportBuildCompleted(exportRes?: ExportRecordResponse) {
runtime_seconds: stateHelper.dockerBuildDurationSeconds
};

core.debug(`exportRes: ${JSON.stringify(exportRes, null, 2)}`);
core.debug(`stateHelper.buildRef: ${stateHelper.buildRef}`);
core.warning(`exportRes: ${JSON.stringify(exportRes, null, 2)}`);
Copy link

Choose a reason for hiding this comment

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

Use core.debug instead of core.warning for logging exportRes and stateHelper.buildRef as they are not warnings.

Suggested change
core.warning(`exportRes: ${JSON.stringify(exportRes, null, 2)}`);
core.debug(`exportRes: ${JSON.stringify(exportRes, null, 2)}`);

@aayushshah15 aayushshah15 force-pushed the 11-25-Only_use_buildRef_if_it_exists branch from b800e6a to 22dd9b8 Compare November 25, 2024 19:54
Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on 22dd9b8 in 25 seconds

More details
  • Looked at 56 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. src/main.ts:86
  • Draft comment:
    Use core.warning with a string message and the error object separately for better error logging.
    core.warning(`Error reporting build completed: ${error.message}`);
  • Reason this comment was not posted:
    Comment was not on a valid diff hunk.

Workflow ID: wflow_0Y84zSf1OeOtZ1Xn


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

3 days left in your free trial, upgrade for $20/seat/month or contact us.

@aayushshah15 aayushshah15 merged commit bdd6696 into master Nov 25, 2024
1 check passed
@aayushshah15 aayushshah15 deleted the 11-25-Only_use_buildRef_if_it_exists branch November 25, 2024 20:00
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.

1 participant