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.

Sign up for GitHub

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

Jump to bottom

Add set_select to set attr value to a select #1153

Merged
merged 2 commits into from
Apr 24, 2023
Merged

Add set_select to set attr value to a select #1153

merged 2 commits into from
Apr 24, 2023

Conversation

healthy-pod
Copy link
Contributor

This code change adds a new command set_select which allows setting the value of an attribute to a call to select and builds the dict using the KVs passed to it.

Example call:

set_select <attr> <key_1> <value_1> <key_n> <value_n>

@google-cla
Copy link

google-cla bot commented Apr 5, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@healthy-pod healthy-pod force-pushed the add_set_select branch 2 times, most recently from 3ce098f to 83543bf Compare April 5, 2023 23:25
@healthy-pod
Copy link
Contributor Author

Ready for review.

@healthy-pod
Copy link
Contributor Author

@vladmos @pmbethe09 @meisterT can you please take a look?

vladmos
vladmos reviewed Apr 11, 2023
View reviewed changes
edit/buildozer_test.go Outdated
func TestCmdSetSelect(t *testing.T) {
buildFile := `foo(
name = "foo",
)`
Copy link
Member

Choose a reason for hiding this comment

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

Could you please add other test cases where 1. a select statement already exists, 2. the attribute already exists but not a select statement?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review, ready for another look.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vladmos friendly ping :)

@healthy-pod healthy-pod force-pushed the add_set_select branch 2 times, most recently from e4a7b29 to 12c84c5 Compare April 13, 2023 12:54
@healthy-pod healthy-pod requested a review from vladmos April 13, 2023 12:56
Add set_select to set attr value to a select
fd73c9a 
This code change adds a new command `set_select` which allows
setting the value of an attribute to a call to `select` and builds
the dict using the KVs passed to it.

Example call:
```
set_select <attr> <key_1> <value_1> <key_n> <value_n>
```
@vladmos vladmos merged commit 43e3add into bazelbuild:master Apr 24, 2023
@healthy-pod healthy-pod mentioned this pull request May 2, 2023
Merged
healthy-pod pushed a commit to healthy-pod/cockroach that referenced this pull request May 2, 2023
*: customize the timeouts used by unit tests in Bazel Essential CI
f024545 
We manage unit tests timeouts at two levels:
1. Bazel timeout, by default [60s,300s,900s,3600s] for [small,medium,large,enormous] targets.
2. Go timeout, set to 5 seconds less than the corresponding Bazel timeout [see cockroachdb#86363].

Previously, unit tests used the same timeouts both when running in `Bazel Essential
CI` and elsewhere. As a result, enormous test targets inherited a timeout of 1 hour
from Bazel's default timeout. This is way beyond the expected time needed by any
test target in `Bazel Essential CI`. We can't change enormous targets to large ones for
two reasons:
1. `Enormous` is also used to indicate the resources needed by a test target.
2. `Enormous` test targets may still need the large timeout when running locally.

To make this possible, we needed to support setting an `attr` value to a `select`
using Buildozer. This was done in bazelbuild/buildtools#1153.

This change only affects the timeout of `enormous` test targets. It however makes it
simple to customize the timeout of other test sizes if desired in the future.

Release note: None
Epic: none
healthy-pod pushed a commit to healthy-pod/cockroach that referenced this pull request May 2, 2023
*: customize the timeouts used by unit tests in Bazel Essential CI
449627f 
We manage unit tests timeouts at two levels:
1. Bazel timeout, by default [60s,300s,900s,3600s] for [small,medium,large,enormous] targets.
2. Go timeout, set to 5 seconds less than the corresponding Bazel timeout [see cockroachdb#86363].

Previously, unit tests used the same timeouts both when running in `Bazel Essential
CI` and elsewhere. As a result, enormous test targets inherited a timeout of 1 hour
from Bazel's default timeout. This is way beyond the expected time needed by any
test target in `Bazel Essential CI`. We can't change enormous targets to large ones for
two reasons:
1. `Enormous` is also used to indicate the resources needed by a test target.
2. `Enormous` test targets may still need the large timeout when running locally.

To make this possible, we needed to support setting an `attr` value to a `select`
using Buildozer. This was done in bazelbuild/buildtools#1153.

This change only affects the timeout of `enormous` test targets. It however makes it
simple to customize the timeout of other test sizes if desired in the future.

Release note: None
Epic: none
craig bot pushed a commit to cockroachdb/cockroach that referenced this pull request May 6, 2023
Merge #102719
53abb2f 
102719: *: customize the timeouts used by unit tests in `Bazel Essential CI` r=rickystewart a=healthy-pod

We manage unit tests timeouts at two levels:
1. Bazel timeout, by default [60s,300s,900s,3600s] for [small,medium,large,enormous] targets.
2. Go timeout, set to 5 seconds less than the corresponding Bazel timeout [see #86363].

Previously, unit tests used the same timeouts both when running in `Bazel Essential CI` and elsewhere. As a result, enormous test targets inherited a timeout of 1 hour from Bazel's default timeout. This is way beyond the expected time needed by any test target in `Bazel Essential CI`. We can't change enormous targets to large ones for two reasons:
1. `Enormous` is also used to indicate the resources needed by a test target.
2. `Enormous` test targets may still need the large timeout when running locally.

To make this possible, we needed to support setting an `attr` value to a `select` using Buildozer. This was done in bazelbuild/buildtools#1153.

This change only affects the timeout of `enormous` test targets. It however makes it simple to customize the timeout of other test sizes if desired in the future.

Release note: None
Epic: none

Co-authored-by: healthy-pod <[email protected]>
apattidb pushed a commit to databricks/buildtools that referenced this pull request May 10, 2024
@healthy-pod @apattidb
Add set_select to set attr value to a select (bazelbuild#1153)
5a77cfd 
This code change adds a new command `set_select` which allows
setting the value of an attribute to a call to `select` and builds
the dict using the KVs passed to it.

Example call:
```
set_select <attr> <key_1> <value_1> <key_n> <value_n>
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pmbethe09 pmbethe09 Awaiting requested review from pmbethe09

@meisterT meisterT Awaiting requested review from meisterT

@vladmos vladmos Awaiting requested review from vladmos

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@healthy-pod @vladmos