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

build(traffic_light_fine_detector): disable downloading artifacts by default #4201

Conversation

esteve
Copy link
Contributor

@esteve esteve commented Jul 7, 2023

Description

Use the DOWNLOAD_ARTIFACTS option to not download artifacts by default, same rationale as #4110.

Tests performed

Not applicable.

Effects on system behavior

Not applicable.

Pre-review checklist for the PR author

The PR author must check the checkboxes below when creating the PR.

In-review checklist for the PR reviewers

The PR reviewers must check the checkboxes below before approval.

Post-review checklist for the PR author

The PR author must check the checkboxes below before merging.

  • There are no open discussions or they are tracked via tickets.

After all checkboxes are checked, anyone who has write access can merge the PR.

@esteve esteve requested a review from Mingyu1991 as a code owner July 7, 2023 11:35
@github-actions github-actions bot added the component:perception Advanced sensor data processing and environment understanding. (auto-assigned) label Jul 7, 2023
@ambroise-arm
Copy link
Contributor

The structure looks like

if(data_exist)
	msg(found)
	if(hash_match)
		msg(ok)
		return
	else
		msg(diff)
		if(DOWNLOAD)
			download
		else
			msg(warn)
else
	if(DOWNLOAD)
		download
	else
		msg(warn)

Would it make sense to simplify as

if(data_exist)
	msg(found)
	if(hash_match)
		msg(ok)
		return
	msg(diff)
if(DOWNLOAD)
	download
else
	msg(warn)

It would avoid the duplication of the download commands.

@esteve esteve force-pushed the disable-artifacts-download-traffic-light-fine-detector branch from 50dc52a to 5b8c05c Compare August 11, 2023 11:20
@esteve esteve requested a review from miursh as a code owner August 11, 2023 11:20
@codecov
Copy link

codecov bot commented Aug 11, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.66% ⚠️

Comparison is base (e1a86f9) 14.90% compared to head (50dc52a) 14.24%.
Report is 7 commits behind head on main.

❗ Current head 50dc52a differs from pull request most recent head 281fc63. Consider uploading reports for the commit 281fc63 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4201      +/-   ##
==========================================
- Coverage   14.90%   14.24%   -0.66%     
==========================================
  Files        1547     1582      +35     
  Lines      106635   108937    +2302     
  Branches    32625    31442    -1183     
==========================================
- Hits        15890    15516     -374     
- Misses      73431    76531    +3100     
+ Partials    17314    16890     -424     
Flag Coverage Δ *Carryforward flag
differential 0.00% <ø> (?)
total 14.24% <ø> (-0.66%) ⬇️ Carriedforward from 632f35f

*This pull request uses carry forward flags. Click here to find out more.

see 396 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@esteve
Copy link
Contributor Author

esteve commented Aug 11, 2023

@ambroise-arm thanks for having a look. I've updated the logic in 5c00581, could you check that it addresses your feedback? Thanks.

Copy link
Contributor

@ambroise-arm ambroise-arm 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!

Copy link
Contributor

@Mingyu1991 Mingyu1991 left a comment

Choose a reason for hiding this comment

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

LGTM

…default

Signed-off-by: Esteve Fernandez <esteve.fernandez@tier4.jp>
Signed-off-by: Esteve Fernandez <esteve.fernandez@tier4.jp>
@esteve esteve force-pushed the disable-artifacts-download-traffic-light-fine-detector branch from 5c00581 to 281fc63 Compare August 17, 2023 10:13
@esteve esteve enabled auto-merge (squash) August 17, 2023 10:13
@miursh miursh added the tag:run-build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) label Sep 22, 2023
@miursh
Copy link
Contributor

miursh commented Sep 22, 2023

@esteve Can we close this PR since #4896 is already merged?

@esteve
Copy link
Contributor Author

esteve commented Sep 22, 2023

@miursh yeah, thanks for the reminder

@esteve esteve closed this Sep 22, 2023
auto-merge was automatically disabled September 22, 2023 08:40

Pull request was closed

@esteve esteve deleted the disable-artifacts-download-traffic-light-fine-detector branch September 22, 2023 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:perception Advanced sensor data processing and environment understanding. (auto-assigned) tag:run-build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants