[System] Adding condition to 0.9.2 package => 0.9.3 by ycombinator · Pull Request #674 · elastic/integrations

ycombinator · 2021-02-08T19:24:02Z

What does this PR do?

This PR effectively performs two steps:

Reverts all changes in the system package back to the 0.9.2 version of the package (commit SHA: 1f5013c
Then adds a condition to the load data stream and bumps up the package version to 0.9.3

However, due to step 1, the diff in this PR is very large. To get a true-r sense of the changes (i.e. just step 2), check out this PR and run git diff 1f5013cef1a441e3d76adcfe687b035c1390617c -- packages/system/.

Immediately following up this PR will be a PR that undoes step 1, therefore bringing the system package back to it's current state in this repo, i.e. as of commit SHA 293d243.

Why is this change important?

The latest released 0.9.x version of the system package is 0.9.2. We need to fix an issue in this series (by adding the condition field). For this, we need to release 0.9.3 of the package with this change.

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.

fearful-symmetry · 2021-02-08T19:29:32Z

@ycombinator so is the plan to let the automation put in the SNAPSHOT PR for this change, then revert?

ycombinator · 2021-02-08T19:30:09Z

@ycombinator so is the plan to let the automation put in the SNAPSHOT PR for this change, then revert?

@fearful-symmetry yep, exactly! That way we end up following all the usual processes and have a record of all changes in the integrations repo as well.

elasticmachine · 2021-02-08T19:30:55Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Build Cause: Pull request #674 updated
Start Time: 2021-02-09T13:25:21.615+0000
Duration: 7 min 23 sec
Commit: 2f8d31b

Test stats 🧪

Test	Results
Failed	0
Passed	89
Skipped	0
Total	89

Trends 🧪

fearful-symmetry

Assuming this is good. In the future we might want to find a less multi-step way to do this, since we're definitely going to see more cases where a stack release is constrained to certain integration releases.

ycombinator · 2021-02-08T19:48:16Z

Welp, we have a problem! CI is failing with this error:

[2021-02-08T19:30:32.915Z] Error: error running package asset tests: could not complete test run: could not install package: could not install package; API status code = 400; response body = {"statusCode":400,"error":"Bad Request","message":"system-0.9.3 is out-of-date and cannot be installed or updated"}

This is because the package registry distribution used for tests by CI contains a newer version of the system package.

Even if we merged this PR with the failing CI 😬, I don't think that's going to help us because I suspect we'll see the same error on the master build that follows. And once that errors out, it won't automatically publish the 0.9.3 version of the system package to https://github.com/elastic/package-storage/tree/snapshot. 😞

I'm wondering now if we should instead make a PR directly to https://github.com/elastic/package-storage/tree/snapshot with a new folder for 0.9.3 (copied from 0.9.2) with the necessary condition change. The down side of this approach is that we won't have a copy of the 0.9.3 system package in the integrations repo and we'd bypass any checks that normally happen on integrations repo PRs. 🤔

@mtojek do you have any opinions or other ideas? Please see the PR description to understand what we are trying to accomplish with this PR.

ycombinator · 2021-02-08T20:28:09Z

I chatted with @jen-huang off-PR about the error we're seeing in CI:

[2021-02-08T19:30:32.915Z] Error: error running package asset tests: could not complete test run: could not install package: could not install package; API status code = 400; response body = {"statusCode":400,"error":"Bad Request","message":"system-0.9.3 is out-of-date and cannot be installed or updated"}

Since we are trying to install the system-0.9.3 package for system testing purposes, I think it's okay to ignore the package version check. IOW, the system test runner needs to be able to install the package version under test so it can run the test, regardless of what other versions of the package are present in the registry.

@jen-huang informed me that this is, in fact, possible with the Fleet API by passing {force: true} in the API's request payload. I'm going to implement this change in elastic-package now: elastic/elastic-package#247.

elasticmachine · 2021-02-09T08:11:07Z

Pinging @elastic/integrations (Team:Integrations)

mtojek

I'm wondering now if we should instead make a PR directly to https://github.com/elastic/package-storage/tree/snapshot with a new folder for 0.9.3 (copied from 0.9.2) with the necessary condition change. The down side of this approach is that we won't have a copy of the 0.9.3 system package in the integrations repo and we'd bypass any checks that normally happen on integrations repo PRs. 🤔

I met a case in the past where there was condition on Kibana version in the package. As no user could install that package I simply modified the broken source in the package-storage. I understand that in this case it's possible to install the package, so your idea is correct, hence LGTM for this PR and the fix in the elastic-package.

I would also re-consider fixing broken packages directly in the package-storage in such emergency cases. It's much easier to handle it operationally.

ycombinator · 2021-02-09T11:52:55Z

Thanks for weighing in, @mtojek .

I understand your point about broken package versions: because such versions cannot be installed anyway by the user, we can assume no one has installed them and, therefore, a fix to them will not require a re-install. So we can safely fix them directly in package-storage.

When you did this in the past, did you also fix the source package version in the integrations repo afterwards? Otherwise there is no record of that fix for that version in the integrations repo, right? If you did fix it in the integrations repo afterwards, how did you do it, considering that the package in the repo @ master could've probably been far ahead in terms of it's changes from the broken version — did you end up with a "messy" PR like the one here + a follow up PR to restore the latest version back in master?

ycombinator · 2021-02-09T12:37:57Z

Just to be clear, the above discussion with @mtojek is not blocking this PR. I'm waiting to rebase this PR on master once #679 is merged.

mtojek · 2021-02-09T12:59:31Z

When you did this in the past, did you also fix the source package version in the integrations repo afterwards? Otherwise there is no record of that fix for that version in the integrations repo, right? If you did fix it in the integrations repo afterwards, how did you do it, considering that the package in the repo @ master could've probably been far ahead in terms of it's changes from the broken version

In my case it was an issue with incompatible import of Kibana dashboards: elastic/package-storage@0767f7a . I fixed it only in the package-storage (must be compatible with Kibana 7.10), because the Apache package's version has been bumped up in Integrations together with bumped up Kibana's constraint (7.11.0 - https://github.com/elastic/integrations/blob/master/packages/apache/manifest.yml#L12). As we're working with latest master in Integrations, I couldn't alter the Git history there.

ycombinator · 2021-02-09T13:05:53Z

As we're working with latest master in Integrations, I couldn't alter the Git history there.

Right, this is what I'm facing with this PR here as well. It's why this PR is so messy (so many changes!) and why we will need a follow up PR to restore the system package as it stands prior to this PR back in master.

I think, in general, we need to have a policy that covers how to fix older versions of packages so we are fixing them consistently and all package authors are aware of it. I'll file a separate issue to take this discussion off this PR.

ycombinator · 2021-02-09T13:17:05Z

So just to recap, the next steps with this PR are as follows:

Now that this PR here is approved and CI is green, I will merge the PR. This will cause the master branch of the integrations repo to temporarily contain 0.9.3 of the system package. CI will build master, which will result in 0.9.3 of the system package being released (i.e. CI will make a PR to the snapshot branch of package-storage with 0.9.3 of the system package).
Once the PR to package-storage resulting from step 1. looks good, I will put up another PR to the master branch of the integrations repo to restore the system package as it stands just prior to this PR here, effectively undoing this PR.

@fearful-symmetry @ChrsMark Please take note of the above, as you two come up as recent editors of the system package. You might want to hold off on further PRs for the system package to the integrations repo until the above two steps are completed. I will post a comment here once both steps are completed and the resulting PRs merged.

This reverts commit b1b35b1.

This reverts commit 6255798.

ycombinator · 2021-02-09T14:09:58Z

I think, in general, we need to have a policy that covers how to fix older versions of packages so we are fixing them consistently and all package authors are aware of it. I'll file a separate issue to take this discussion off this PR.

Created issue for discussion: #682.

ycombinator · 2021-02-09T14:13:55Z

So just to recap, the next steps with this PR are as follows:

Now that this PR here is approved and CI is green, I will merge the PR. This will cause the master branch of the integrations repo to temporarily contain 0.9.3 of the system package. CI will build master, which will result in 0.9.3 of the system package being released (i.e. CI will make a PR to the snapshot branch of package-storage with 0.9.3 of the system package).

This is done now. The PR to package-storage is up: elastic/package-storage#869.

Once the PR to package-storage resulting from step 1. looks good, I will put up another PR to the master branch of the integrations repo to restore the system package as it stands just prior to this PR here, effectively undoing this PR.

This is also done now. The PR to revert this PR here is up: #681.

@mtojek @fearful-symmetry I've requested your reviews on both PRs above.

…681) This reverts commit 6255798.

ycombinator requested a review from fearful-symmetry February 8, 2021 19:24

ycombinator changed the title ~~Adding condition~~ [System] Adding condition to 0.9.2 package => 0.9.3 Feb 8, 2021

ycombinator requested a review from mtojek February 8, 2021 19:24

fearful-symmetry approved these changes Feb 8, 2021

View reviewed changes

ycombinator mentioned this pull request Feb 8, 2021

"error: Metricbeat FAILED" errors under Logs tab on enrolling 7.11 Windows agent with 7.10 default policy on upgraded kibana(7.10.2-7.11.0). elastic/beats#23812

Closed

andresrc added the Team:Integrations Label for the Integrations team label Feb 9, 2021

mtojek approved these changes Feb 9, 2021

View reviewed changes

This was referenced Feb 9, 2021

Allow installing older versions of the package elastic/elastic-package#247

Merged

Updates the dependency on elastic-package #679

Merged

Adding condition

300e96c

ycombinator force-pushed the system-093 branch from d4234d7 to 300e96c Compare February 9, 2021 13:00

Revert "Add system.hostfs setting (elastic#673)"

2f8d31b

This reverts commit b1b35b1.

ycombinator merged commit 6255798 into elastic:master Feb 9, 2021

ycombinator deleted the system-093 branch February 9, 2021 13:34

ycombinator added a commit that referenced this pull request Feb 9, 2021

Revert "[System] Adding condition to 0.9.2 package => 0.9.3 (#674)"

ce53d82

This reverts commit 6255798.

ycombinator mentioned this pull request Feb 9, 2021

Revert "[System] Adding condition to 0.9.2 package => 0.9.3" #681

Merged

ycombinator added a commit that referenced this pull request Feb 9, 2021

Revert "[System] Adding condition to 0.9.2 package => 0.9.3 (#674)" (#…

513ad6f

…681) This reverts commit 6255798.

Conversation

ycombinator commented Feb 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Why is this change important?

Checklist

Uh oh!

fearful-symmetry commented Feb 8, 2021

Uh oh!

ycombinator commented Feb 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Feb 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💚 Build Succeeded

Build stats

Test stats 🧪

Trends 🧪

Uh oh!

fearful-symmetry left a comment

Choose a reason for hiding this comment

Uh oh!

ycombinator commented Feb 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ycombinator commented Feb 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Feb 9, 2021

Uh oh!

mtojek left a comment

Choose a reason for hiding this comment

Uh oh!

ycombinator commented Feb 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ycombinator commented Feb 9, 2021

Uh oh!

mtojek commented Feb 9, 2021

Uh oh!

ycombinator commented Feb 9, 2021

Uh oh!

ycombinator commented Feb 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ycombinator commented Feb 9, 2021

Uh oh!

ycombinator commented Feb 9, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ycombinator commented Feb 8, 2021 •

edited

Loading

ycombinator commented Feb 8, 2021 •

edited

Loading

elasticmachine commented Feb 8, 2021 •

edited

Loading

ycombinator commented Feb 8, 2021 •

edited

Loading

ycombinator commented Feb 8, 2021 •

edited

Loading

ycombinator commented Feb 9, 2021 •

edited

Loading

ycombinator commented Feb 9, 2021 •

edited

Loading