-
Notifications
You must be signed in to change notification settings - Fork 3
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
Remove run ID and introduce deploy ID #93
Conversation
…ling, remove run id from NameUtils
…id -> runid in logs so there are no immediate compatibility issues
So, there no
I did a test run via GitHub Actions and there we still assign
|
Correct, now there's only a deploy id, which you can set when you deploy a new network.
Yes, if nothing is set. You can still customize it with
Yes (see above).
The runid in the Pod labels should already be correct, and match what's in the status.
Correct, I didn't change the external facing attribute names (except for the env var 😬) - sorry for not mentioning that sooner. They are still called runid both in the status log and the Prometheus labels, even though they are now technically deploy IDs and are no longer allowed to diverge.
Yeah, for now what will happen is that if you don't set DEPLOYID then it will simply generate it automatically, using a very similar strategy to what you have in the github action. |
Tests/DistTestCore/Logs/StatusLog.cs
Outdated
@@ -26,7 +28,7 @@ public void ConcludeTest(string resultStatus, TimeSpan testDuration, Dictionary< | |||
public void ConcludeTest(string resultStatus, string testDuration, Dictionary<string, string> data) | |||
{ | |||
data.Add("timestamp", DateTime.UtcNow.ToString("o")); | |||
data.Add("runid", NameUtils.GetRunId()); | |||
data.Add("runid", deployId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
key is still "runid". Should it be "deployid"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I was delaying changing the actual label to sync up with @veaceslavdoina, maybe now is the time.
Tests/DistTestCore/TestLifecycle.cs
Outdated
@@ -76,7 +78,7 @@ public void OnContainersStopped(RunningContainers rc) | |||
public void OnContainerRecipeCreated(ContainerRecipe recipe) | |||
{ | |||
recipe.PodLabels.Add("tests-type", TestsType); | |||
recipe.PodLabels.Add("runid", NameUtils.GetRunId()); | |||
recipe.PodLabels.Add("runid", deployId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here!
Tools/CodexNetDeployer/K8sHook.cs
Outdated
@@ -27,7 +29,7 @@ public void OnContainersStopped(RunningContainers rc) | |||
public void OnContainerRecipeCreated(ContainerRecipe recipe) | |||
{ | |||
recipe.PodLabels.Add("tests-type", testsTypeLabel); | |||
recipe.PodLabels.Add("runid", NameUtils.GetRunId()); | |||
recipe.PodLabels.Add("runid", deployId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one more!
This PR removes the notion of a run id and replaces it with a deploy id in continuous tests. Deploy ids can be set at deploy time only (duh), and will be picked up by the test runner from the deploy file on subsequent runs of the continuous test runner. As a consequence, the deploy id becomes a deployer parameter, and can no longer be overridden at the runner. For non-continuous tests, the deploy ID is created on-the-fly.