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

drtprod: introduce target dependency #131942

Merged
merged 1 commit into from
Oct 25, 2024

Conversation

nameisbhaskar
Copy link
Contributor

@nameisbhaskar nameisbhaskar commented Oct 4, 2024

Today there is no way to define dependency on other targets. This is needed in certain cases. e.g. if we want to copy certificates after both the clusters are ready.
This PR also resolves an issue with roachprod command where the stdin input was not considered. Now, the command is executed with the interactive mode.

Some more changes done for 150 node:
schema_change is created without cron entry as we need manual control on it

Epic: None
Release note: None

Copy link

blathers-crl bot commented Oct 4, 2024

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@nameisbhaskar nameisbhaskar force-pushed the add-target-dependency branch 2 times, most recently from 4684a12 to 34843a3 Compare October 5, 2024 00:26
@nameisbhaskar nameisbhaskar marked this pull request as ready for review October 5, 2024 00:31
@nameisbhaskar nameisbhaskar requested a review from a team as a code owner October 5, 2024 00:31
@nameisbhaskar nameisbhaskar requested review from DarrylWong and vidit-bhat and removed request for a team October 5, 2024 00:31
@nameisbhaskar nameisbhaskar force-pushed the add-target-dependency branch 4 times, most recently from e484d03 to c5816ba Compare October 9, 2024 12:58
@nameisbhaskar nameisbhaskar force-pushed the add-target-dependency branch 6 times, most recently from d626b38 to 8dd52ad Compare October 17, 2024 07:45
@nameisbhaskar nameisbhaskar force-pushed the add-target-dependency branch 4 times, most recently from 27fdecf to 9609266 Compare October 20, 2024 16:04
@nameisbhaskar nameisbhaskar marked this pull request as draft October 20, 2024 19:37
@nameisbhaskar
Copy link
Contributor Author

moving to draft to wait for other changes of tpcc and tpch to get merged.

@vidit-bhat vidit-bhat marked this pull request as ready for review October 21, 2024 08:25
@nameisbhaskar nameisbhaskar force-pushed the add-target-dependency branch 3 times, most recently from 1116480 to b579689 Compare October 25, 2024 08:25
pkg/cmd/drtprod/cli/commands/yamlprocessor.go Outdated Show resolved Hide resolved
pkg/cmd/drtprod/cli/commands/yamlprocessor.go Outdated Show resolved Hide resolved
pkg/cmd/drtprod/helpers/utils.go Outdated Show resolved Hide resolved
Copy link
Contributor

@sambhav-jain-16 sambhav-jain-16 left a comment

Choose a reason for hiding this comment

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

Thanks @nameisbhaskar for the PR!
I have 2 nit comments. Apart from it, LGTM !

pkg/cmd/drtprod/cli/commands/yamlprocessor.go Outdated Show resolved Hide resolved
pkg/cmd/drtprod/cli/commands/yamlprocessor.go Outdated Show resolved Hide resolved
@nameisbhaskar nameisbhaskar force-pushed the add-target-dependency branch 2 times, most recently from ee550f9 to 1ffe6cf Compare October 25, 2024 11:04
@nameisbhaskar
Copy link
Contributor Author

Thanks @vidit-bhat and @sambhav-jain-16 for reviewing the PR. I have addressed the comments. PTAL

Copy link
Contributor

@sambhav-jain-16 sambhav-jain-16 left a comment

Choose a reason for hiding this comment

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

LGTM !
Thanks @nameisbhaskar for the changes !

Copy link
Contributor

@vidit-bhat vidit-bhat left a comment

Choose a reason for hiding this comment

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

LGTM

Today there is no way to define dependency on other targets. This is needed in certain cases. e.g. if we want to copy certificates after both the clusters are ready.
This PR also resolves an issue with roachprod command where the stdin input was not considered. Now, the command is executed with the interactive mode.

Some more changes done for 150 node:
schema_change is created without cron entry as we need manual control on it

Epic: None
Release note: None
@nameisbhaskar
Copy link
Contributor Author

Thanks @vidit-bhat and @sambhav-jain-16 for approving the changes!

@nameisbhaskar
Copy link
Contributor Author

bors r=@sambhav-jain-16,@vidit-bhat

@craig craig bot merged commit 2c02451 into cockroachdb:master Oct 25, 2024
22 of 23 checks passed
@nameisbhaskar nameisbhaskar deleted the add-target-dependency branch October 25, 2024 12:02
msbutler pushed a commit to msbutler/cockroach that referenced this pull request Oct 25, 2024
131942: drtprod: introduce target dependency r=sambhav-jain-16,vidit-bhat a=nameisbhaskar

Today there is no way to define dependency on other targets. This is needed in certain cases. e.g. if we want to copy certificates after both the clusters are ready.
This PR also resolves an issue with roachprod command where the stdin input was not considered. Now, the command is executed with the interactive mode.

Some more changes done for 150 node:
schema_change is created without cron entry as we need manual control on it

Epic: None
Release note: None

Co-authored-by: Bhaskarjyoti Bora <[email protected]>
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.

4 participants