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

Impacted area based PR test. #15666

Open
wants to merge 25 commits into
base: master
Choose a base branch
from

Conversation

yutongzhang-microsoft
Copy link
Contributor

Description of PR

Summary:
Fixes # (issue)

Type of change

  • Bug fix
  • Testbed and Framework(new/improvement)
  • Test case(new/improvement)

Back port request

  • 202012
  • 202205
  • 202305
  • 202311
  • 202405

Approach

What is the motivation for this PR?

How did you do it?

How did you verify/test it?

Any platform specific information?

Supported testbed topology if it's a new test case?

Documentation

Comment on lines +98 to +99
MIN_WORKER: $(INSTANCE_NUMBER)
MAX_WORKER: $(INSTANCE_NUMBER)
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like they can only get from environment variables, do they have default value?

Copy link
Contributor Author

@yutongzhang-microsoft yutongzhang-microsoft Dec 4, 2024

Choose a reason for hiding this comment

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

It's not a environment variable, it's the variable set in calculate-instance-numbers.yml

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, if the pre-step fail, do we have an default value of instance_number to always run the test like before.
As well as scripts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think if the pre-step fail, we will not continue the pipeline, just fail here. As the default instance number is not suitable, and it may cause too much time running PR testing.

.azure-pipelines/impacted_area_testing/get_test_scripts.py Outdated Show resolved Hide resolved
Comment on lines 22 to 33
ingest_cluster = os.getenv("TEST_REPORT_QUERY_KUSTO_CLUSTER_BACKUP")
access_token = os.getenv('ACCESS_TOKEN', None)

if not ingest_cluster or not access_token:
raise RuntimeError(
"Could not load Kusto Credentials from environment")
else:
kcsb = KustoConnectionStringBuilder.with_aad_application_token_authentication(ingest_cluster,
access_token) # noqa F841

client = KustoClient(kcsb)

Copy link
Contributor

Choose a reason for hiding this comment

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

If query from kusto fail, post action will be blocked, right? So, suggest to enhance it with setting default instance_num for the calculate_instance_number task.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have set the default instance number MAX_INSTANCE_NUMBER

azure-pipelines.yml Outdated Show resolved Hide resolved
@yutongzhang-microsoft yutongzhang-microsoft force-pushed the yutongzhang/pr_impacted_area branch 4 times, most recently from c9d0353 to 46f8aef Compare December 11, 2024 06:01
@wangxin
Copy link
Collaborator

wangxin commented Dec 11, 2024

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@yutongzhang-microsoft yutongzhang-microsoft force-pushed the yutongzhang/pr_impacted_area branch from 5200cd3 to bcf1534 Compare December 13, 2024 07:45
@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@yutongzhang-microsoft yutongzhang-microsoft force-pushed the yutongzhang/pr_impacted_area branch from f9e5752 to 2e8d8c4 Compare December 13, 2024 08:47
@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

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