Skip to content

Conversation

zhiyuanliang-ms
Copy link
Contributor

@zhiyuanliang-ms zhiyuanliang-ms commented Sep 11, 2024

Why this PR?

The repo folder structure has been changed to accommodate more packages in the future.

@zhiyuanliang-ms zhiyuanliang-ms force-pushed the zhiyuanliang/telemetry-support branch 3 times, most recently from 6b802cf to c397db9 Compare September 11, 2024 18:50
@zhiyuanliang-ms zhiyuanliang-ms changed the base branch from main to preview September 11, 2024 18:53
@zhiyuanliang-ms zhiyuanliang-ms changed the title Telemetry Support Add basic telemetry support Sep 11, 2024
@zhiyuanliang-ms zhiyuanliang-ms marked this pull request as ready for review September 11, 2024 19:08
SDK_DIR="$PROJECT_BASE_DIR/sdk"

# Loop through each package in the sdk folder.
for PACKAGE_DIR in "$SDK_DIR"/*; do
Copy link
Member

Choose a reason for hiding this comment

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

this script is all-in-one. Suppose you will have multiple packages later, do you want to create RC bits and release for all of them everytime? If so, that's fine. If you will release one of these packages alone, I would suggest you accept package_dir as a parameter of this script.

Copy link
Member

Choose a reason for hiding this comment

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

It indeed takes me a while to find where telemetry support is added, mixed with the file renaming. A general suggestion is to make the file moving into a separate PR and focus on telemetry part in this PR. That would make it easier to review.

Copy link
Member

Choose a reason for hiding this comment

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

...and I have to double check whether there's additional changes in each renamed file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Revert the change. Only keep the change of folder structure change in this PR.

@zhiyuanliang-ms zhiyuanliang-ms force-pushed the zhiyuanliang/telemetry-support branch from 53085fc to 75bb990 Compare September 12, 2024 02:55
@zhiyuanliang-ms zhiyuanliang-ms changed the title Add basic telemetry support Adjust folder structure Sep 12, 2024
@zhiyuanliang-ms zhiyuanliang-ms force-pushed the zhiyuanliang/telemetry-support branch from 75bb990 to e953caf Compare September 12, 2024 03:29
@zhiyuanliang-ms zhiyuanliang-ms merged commit 525092d into preview Sep 12, 2024
3 checks passed
@zhiyuanliang-ms zhiyuanliang-ms deleted the zhiyuanliang/telemetry-support branch September 12, 2024 04:49
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.

2 participants