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

Test each integration with the oldest supported version #1541

Merged
merged 29 commits into from
Aug 26, 2021

Conversation

jsoriano
Copy link
Member

@jsoriano jsoriano commented Aug 18, 2021

What does this PR do?

Test each package with the oldest supported version of the stack. This will help making the builds a bit more deterministic, while ensuring that no changes are introduced that break the condition of supported version.

If the version doesn't exist yet, it will try to use the closer snapshot, for example if 7.14.1 doesn't exist, it will try to use 7.14.1-SNAPSHOT or 7.x-SNAPSHOT, this will allow to develop integrations with unreleased features.

Some packages using 7.14.0 have been modified to require at least 7.14.1 as they were affected by elastic/beats#27299.

Checklist

  • I have reviewed tips for building integrations and this pull request is aligned with them.
  • I have verified that all data streams collect metrics or logs.
  • I have added an entry to my package's changelog.yml file.
  • If I'm introducing a new feature, I have modified the Kibana version constraint in my package's manifest.yml file to point to the latest Elastic stack release (e.g. ^7.13.0).

Author's Checklist

How to test this PR locally

Related issues

Screenshots

@jsoriano jsoriano force-pushed the integrations-stack-version branch 2 times, most recently from f708e3c to 1b79f82 Compare August 18, 2021 12:15
@elasticmachine
Copy link

elasticmachine commented Aug 18, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2021-08-26T10:35:16.425+0000

  • Duration: 70 min 37 sec

  • Commit: e1722f5

Test stats 🧪

Test Results
Failed 0
Passed 2896
Skipped 3
Total 2899

Trends 🧪

Image of Build Times

Image of Tests

@jsoriano jsoriano force-pushed the integrations-stack-version branch 4 times, most recently from ce1d34c to beaf3e1 Compare August 18, 2021 15:58
@jsoriano jsoriano marked this pull request as ready for review August 23, 2021 17:00
@elasticmachine
Copy link

Pinging @elastic/integrations (Team:Integrations)

@elasticmachine
Copy link

Pinging @elastic/security-external-integrations (Team:Security-External Integrations)

Copy link
Member

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

I really like the idea that we test with the oldest version supported.

Later on, I wonder if we could also do a test run with the newest version supported. This might be unstable and maybe only run ones every day / week to show where we are standing.

description: This Elastic integration collects Blue Coat Director logs
categories: ["network", "security"]
release: experimental
license: basic
type: integration
conditions:
kibana.version: "^7.14.0"
kibana.version: "^7.14.1"
Copy link
Member

Choose a reason for hiding this comment

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

Is this change by itention? Same for the other packaegs?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this is intentional, these packages fail to install in 7.14.0, probably due to elastic/beats#27299.

Copy link
Member

Choose a reason for hiding this comment

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

Got it. Sounds like we should start working on elastic_agent.version: ... as this one is not Kibana related.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I think we are going to need conditions for elastic agent soon, but this will imply more changes.

@jsoriano
Copy link
Member Author

Later on, I wonder if we could also do a test run with the newest version supported. This might be unstable and maybe only run ones every day / week to show where we are standing.

Yep, agree, I was thinking on adding a jenkins parameter so we can launch all the tests with an specific version, this would allow for example to run the tests against a build candidate. This could be run with master/7.x on a daily basis.

.ci/Jenkinsfile Outdated Show resolved Hide resolved
.ci/Jenkinsfile Outdated
}

def findStackVersion(String versionCondition) {
def version = versionCondition.substring(1)
Copy link
Member

@endorama endorama Aug 24, 2021

Choose a reason for hiding this comment

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

Is this line supposed to remove the initial v from the version string? I would add a note about it's behaviour.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree, this is a bit obscure. Victor is moving this function to the shared library and we are being more explicit about this there (explicitly checking that first character is ^, documenting that we only support this operator here and testing this expectation). You can find it here: elastic/apm-pipeline-library#1248

.ci/Jenkinsfile Outdated
Comment on lines 254 to 255
// Old minors may not be available in artifacts-api, if it is older
// than the others in the same major, return the version as is.
Copy link
Member

Choose a reason for hiding this comment

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

It is not clear to me what is happening here and why returning the version as is is the correct choice.
Does artifacts-api delete all minors apart from latest? (so for example it keeps 7.x and not 7.x-1)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, older versions are not available in this API, you can see it here: https://artifacts-api.elastic.co/v1/versions. For example 7.13.0 is not there now.

In these cases I am assuming that these older versions existed, and their docker images are available.

.ci/Jenkinsfile Outdated Show resolved Hide resolved
.ci/Jenkinsfile Outdated Show resolved Hide resolved
jsoriano and others added 4 commits August 24, 2021 17:29
Co-authored-by: Victor Martinez <victormartinezrubio@gmail.com>
Co-authored-by: Victor Martinez <victormartinezrubio@gmail.com>
if (manifest != null && manifest["conditions"] != null) {
def kibanaVersionCondition = manifest["conditions"]["kibana.version"]
if (kibanaVersionCondition != null) {
def version = findOldestSupportedVersion(versionCondition: kibanaVersionCondition)
Copy link
Contributor

Choose a reason for hiding this comment

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

I cannot find findOldestSupportedVersion, where is it defined?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

We use a shared library for jenkinsfiles, this function has been moved there: elastic/apm-pipeline-library#1248

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@aspacca
Copy link
Contributor

aspacca commented Aug 26, 2021

prepareStack() LGTM, cannot say about conditions: kibana.version bump, I assume it is correct as well

@jsoriano
Copy link
Member Author

cannot say about conditions: kibana.version bump, I assume it is correct as well

This is needed for packages that fail to install in 7.14.0 (see this comment #1541 (comment)).

@jsoriano jsoriano merged commit f50eb7c into elastic:master Aug 26, 2021
@jsoriano jsoriano deleted the integrations-stack-version branch August 26, 2021 13:18
eyalkraft pushed a commit to build-security/integrations that referenced this pull request Mar 30, 2022
Test each package with the oldest supported version of the stack. This
will help making the builds a bit more deterministic, while ensuring that
no changes are introduced that break the condition of supported version.

If the version doesn't exist yet, it will try to use the closer snapshot, for
example if 7.14.1 doesn't exist, it will try to use 7.14.1-SNAPSHOT or
7.x-SNAPSHOT, this will allow to develop integrations with unreleased
features.

Some packages using 7.14.0 have been modified to require at least 7.14.1
as they were affected by elastic/beats#27299.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:Integrations Label for the Integrations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants