Clarify purpose of `.agent-versions` file #5522

ycombinator · 2024-09-11T15:30:20Z

What does this PR do?

This PR clarifies the purpose of the .agent-versions file. It does so by making the following changes:

Moves and renames the file from .agent-versions.json to testing/integration/testdata/.upgrade-test-agent-versions.yml and adjust the contents to match. The reason for using YAML is so we can add a header comment about the contents of the file in the file itself.
Updates the upgrade integration tests that use this file to reference the new one.
Updates the commit message in PRs created by automation to keep this file updated.

Before this PR

Before this PR, we had an .agent-versions.json file and it looked like this:

{
  "testVersions": [
    "8.15.2-SNAPSHOT",
    "8.15.1",
    "8.15.0",
    "7.17.25-SNAPSHOT"
  ]
}

After this PR

After this PR, we instead have an testing/integration/testdata/.upgrade-test-agent-versions.yml file and it looks like this:

# This file is generated automatically. Please do not manually edit it.

# The testVersions list in this file specifies Elastic Agent versions to be used as
# the starting (pre-upgrade) or ending (post-upgrade) versions of Elastic Agent in
# upgrade integration tests.

testVersions:
  - 8.15.2-SNAPSHOT
  - 8.15.1
  - 8.15.0
  - 7.17.25-SNAPSHOT

Why is it important?

To make it easier to understand the purpose of the .agent-versions file and to prevent accidental manual edits to it.

How to test this PR locally

Create a dummy, valid testing/integration/testdata/.upgrade-test-agent-versions.yml file.
Run integration:updateVersions.
Make sure the testing/integration/testdata/.upgrade-test-agent-versions.yml file is updated with the expected contents.

mergify · 2024-09-11T15:31:11Z

This pull request does not have a backport label. Could you fix it @ycombinator? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

backport-./d./d is the label to automatically backport to the 8./d branch. /d is the digit

mergify · 2024-09-11T15:31:12Z

backport-v8.x has been added to help with the transition to the new branch 8.x.

elasticmachine · 2024-09-11T16:03:12Z

Pinging @elastic/elastic-agent-control-plane (Team:Elastic-Agent-Control-Plane)

pchila

LGTM

rdner · 2024-09-11T18:53:23Z

.github/workflows/bump-agent-versions.sh

@@ -22,7 +22,7 @@ else
    git add .agent-versions.json .package-version

    nl=$'\n' # otherwise the new line character is not recognized properly
-    commit_desc="These files are used for picking agent versions in integration tests.${nl}${nl}The content is based on responses from https://www.elastic.co/api/product_versions and https://snapshots.elastic.co${nl}${nl}The current update is generated based on the following requirements:${nl}${nl}Package version: ${package_version}${nl}${nl}\`\`\`json${nl}${version_requirements}${nl}\`\`\`"
+    commit_desc="These files are used for picking the starting (pre-upgrade) agent versions in upgrade integration tests.${nl}${nl}The content is based on responses from https://www.elastic.co/api/product_versions and https://snapshots.elastic.co${nl}${nl}The current update is generated based on the following requirements:${nl}${nl}Package version: ${package_version}${nl}${nl}\`\`\`json${nl}${version_requirements}${nl}\`\`\`"


This is not entirely true, the upgrade tests run both directions and these versions are not used in just upgrade tests.

Thanks, I see now that the versions from this file are not just used as starting versions in the upgrade test. For example, TestStandaloneUpgradeRetryDownload calls upgradetest.PreviousMinor() to set the end version and that function ends up looking at the versions from the file. So I will fix the text in this PR accordingly.

But I'm not seeing where the versions from this file are being used outside of upgrade tests. Could you point me to an example?

@ycombinator yes, I meant the usage of upgradetest.PreviousMinor() in other tests, there is a few of them https://github.com/search?q=repo%3Aelastic%2Felastic-agent%20upgradetest.PreviousMinor()&type=code

swiatekm · 2024-09-12T08:38:44Z

Since this file is only used for tests, can we move it to the testing directory, and maybe change the name to be more indicative of the purpose? Say .upgrade-test-agent-versions.yaml? In general, it almost seems like an implementation detail of the upgradetest package, minus the fact that a mage target is used to update it (I assume that's why it's a separate file and not just some constants in Go code.).

ycombinator · 2024-09-12T13:21:33Z

Since this file is only used for tests, can we move it to the testing directory, and maybe change the name to be more indicative of the purpose? Say .upgrade-test-agent-versions.yaml? In general, it almost seems like an implementation detail of the upgradetest package, minus the fact that a mage target is used to update it (I assume that's why it's a separate file and not just some constants in Go code.).

I'm a +1 to this idea. The generic name of the file and it's current location has confused me in the past, which is part of the reason I raised this PR to switch to YAML so we could at least add comments in the file.

Other reviewers (@rdner @pchila @AndersonQ @blakerouse @cmacknz), any objections to this move? I'll make sure that everyone on the team is aware of it, in case they go looking for the file.

pchila · 2024-09-12T13:28:13Z

I'm a +1 to this idea. The generic name of the file and it's current location has confused me in the past, which is part of the reason I raised this PR to switch to YAML so we could at least add comments in the file.

Other reviewers (@rdner @pchila @AndersonQ @blakerouse @cmacknz), any objections to this move? I'll make sure that everyone on the team is aware of it, in case they go looking for the file.

If we are discussing moving files, .agent-versions should live under testing/integration/testdata (along with a good rename to make it explicit what it's for) since it's data for the upgrade tests to use.

In any case I am in favor of moving the file away from the repo root and/or renaming. 👍

rdner · 2024-09-12T13:44:39Z

I'm fine with moving the file. It might require changes in the automation code here https://github.com/elastic/elastic-agent/blob/main/.github/workflows/bump-agent-versions.sh

rdner · 2024-09-12T13:45:44Z

@ycombinator actually this script requires changes even for this original PR here

elastic-agent/.github/workflows/bump-agent-versions.sh

Line 6 in 80ad645

changes=$(git status -s -uno .agent-versions.json .package-version)

I missed that 🙂

ycombinator · 2024-09-13T00:25:31Z

Based on the discussion, I've moved the .agent-versions file to testing/integration/testdata/.upgrade-test-agent-versions.yml and updated code to look at this new location and filename. Please re-review when you get a chance. Thanks!

cmacknz · 2024-09-17T21:00:24Z

testing/integration/testdata/.upgrade-test-agent-versions.yml

@@ -0,0 +1,12 @@
+# This file is generated automatically. Please do not manually edit it.


We do manually edit this occasionally when we need to temporarily block a version because of a bug, but in general that is the exception.

elastic-sonarqube · 2024-09-20T01:30:13Z

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarQube

* Clarify commit message * Convert file to YML and add header comment * Reintroducing rand/v2 import cleaned up by IDE * Use YAML v3 * Replace .agent-versions.json with .agent-versions.yml * Running mage fmt * Adding a newline in the header comment * Fix comment * Fix commit message * Look for changes in new file * Moving + renaming agent versions file * Updating code to refer to new file location + name * Running mage integration:updateVersions (cherry picked from commit 2feeb36) # Conflicts: # .agent-versions.json

* Clarify commit message * Convert file to YML and add header comment * Reintroducing rand/v2 import cleaned up by IDE * Use YAML v3 * Replace .agent-versions.json with .agent-versions.yml * Running mage fmt * Adding a newline in the header comment * Fix comment * Fix commit message * Look for changes in new file * Moving + renaming agent versions file * Updating code to refer to new file location + name * Running mage integration:updateVersions (cherry picked from commit 2feeb36) # Conflicts: # .agent-versions.json Co-authored-by: Shaunak Kashyap <[email protected]>

* Clarify commit message * Convert file to YML and add header comment * Reintroducing rand/v2 import cleaned up by IDE * Use YAML v3 * Replace .agent-versions.json with .agent-versions.yml * Running mage fmt * Adding a newline in the header comment * Fix comment * Fix commit message * Look for changes in new file * Moving + renaming agent versions file * Updating code to refer to new file location + name * Running mage integration:updateVersions

mergify bot assigned ycombinator Sep 11, 2024

mergify bot added the backport-8.x Automated backport to the 8.x branch with mergify label Sep 11, 2024

ycombinator added the skip-changelog label Sep 11, 2024

ycombinator requested review from rdner and cmacknz September 11, 2024 16:02

ycombinator added the Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team label Sep 11, 2024

ycombinator marked this pull request as ready for review September 11, 2024 16:03

ycombinator requested a review from a team as a code owner September 11, 2024 16:03

ycombinator requested review from pchila and blakerouse September 11, 2024 16:03

pchila approved these changes Sep 11, 2024

View reviewed changes

rdner reviewed Sep 11, 2024

View reviewed changes

rdner approved these changes Sep 12, 2024

View reviewed changes

AndersonQ approved these changes Sep 12, 2024

View reviewed changes

ycombinator requested review from AndersonQ, rdner and pchila September 13, 2024 00:25

swiatekm approved these changes Sep 13, 2024

View reviewed changes

AndersonQ approved these changes Sep 13, 2024

View reviewed changes

pchila approved these changes Sep 16, 2024

View reviewed changes

ycombinator force-pushed the clarify-agent-version-automation branch from 51db753 to a47ffb2 Compare September 16, 2024 14:26

cmacknz reviewed Sep 17, 2024

View reviewed changes

cmacknz approved these changes Sep 17, 2024

View reviewed changes

ycombinator added 13 commits September 19, 2024 17:39

Clarify commit message

2327350

Convert file to YML and add header comment

6edb8f9

Reintroducing rand/v2 import cleaned up by IDE

7734a81

Use YAML v3

a63da36

Replace .agent-versions.json with .agent-versions.yml

70caf29

Running mage fmt

ef48de6

Adding a newline in the header comment

0418a5a

Fix comment

7e99313

Fix commit message

06d314c

Look for changes in new file

e042019

Moving + renaming agent versions file

e42c832

Updating code to refer to new file location + name

884d0d2

Running mage integration:updateVersions

b196b17

ycombinator force-pushed the clarify-agent-version-automation branch from a47ffb2 to b196b17 Compare September 20, 2024 00:39

ycombinator merged commit 2feeb36 into elastic:main Sep 20, 2024
13 checks passed

ycombinator deleted the clarify-agent-version-automation branch September 20, 2024 04:14

mergify bot mentioned this pull request Sep 20, 2024

[8.x](backport #5522) Clarify purpose of .agent-versions file #5575

Merged

ycombinator mentioned this pull request Sep 20, 2024

[main][Automation] Update versions #5552

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clarify purpose of `.agent-versions` file #5522

Clarify purpose of `.agent-versions` file #5522

ycombinator commented Sep 11, 2024 •

edited

Loading

mergify bot commented Sep 11, 2024

mergify bot commented Sep 11, 2024

elasticmachine commented Sep 11, 2024

pchila left a comment

rdner Sep 11, 2024 •

edited

Loading

ycombinator Sep 11, 2024

rdner Sep 12, 2024

swiatekm commented Sep 12, 2024

ycombinator commented Sep 12, 2024 •

edited

Loading

pchila commented Sep 12, 2024 •

edited

Loading

rdner commented Sep 12, 2024

rdner commented Sep 12, 2024

ycombinator commented Sep 13, 2024

cmacknz Sep 17, 2024

elastic-sonarqube bot commented Sep 20, 2024

		@@ -0,0 +1,12 @@
		# This file is generated automatically. Please do not manually edit it.

Clarify purpose of .agent-versions file #5522

Clarify purpose of .agent-versions file #5522

Conversation

ycombinator commented Sep 11, 2024 • edited Loading

What does this PR do?

Before this PR

After this PR

Why is it important?

How to test this PR locally

mergify bot commented Sep 11, 2024

mergify bot commented Sep 11, 2024

elasticmachine commented Sep 11, 2024

pchila left a comment

Choose a reason for hiding this comment

rdner Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

ycombinator Sep 11, 2024

Choose a reason for hiding this comment

rdner Sep 12, 2024

Choose a reason for hiding this comment

swiatekm commented Sep 12, 2024

ycombinator commented Sep 12, 2024 • edited Loading

pchila commented Sep 12, 2024 • edited Loading

rdner commented Sep 12, 2024

rdner commented Sep 12, 2024

ycombinator commented Sep 13, 2024

cmacknz Sep 17, 2024

Choose a reason for hiding this comment

elastic-sonarqube bot commented Sep 20, 2024

Quality Gate passed

Clarify purpose of `.agent-versions` file #5522

Clarify purpose of `.agent-versions` file #5522

ycombinator commented Sep 11, 2024 •

edited

Loading

rdner Sep 11, 2024 •

edited

Loading

ycombinator commented Sep 12, 2024 •

edited

Loading

pchila commented Sep 12, 2024 •

edited

Loading