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

[Apache] Use new labels for license and subscription #3816

Merged
merged 2 commits into from
Jul 28, 2022

Conversation

jsoriano
Copy link
Member

What does this PR do?

license field is deprecated as of elastic/package-spec#355, use the new fields in the apache package.

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.
  • I have verified that Kibana version constraints are current according to guidelines.

Related issues

@jsoriano jsoriano requested a review from a team July 24, 2022 10:39
@jsoriano jsoriano requested a review from a team as a code owner July 24, 2022 10:39
@jsoriano jsoriano self-assigned this Jul 24, 2022
@elasticmachine
Copy link

elasticmachine commented Jul 24, 2022

💚 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: 2022-07-25T08:44:43.482+0000

  • Duration: 18 min 28 sec

Test stats 🧪

Test Results
Failed 0
Passed 45
Skipped 0
Total 45

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

@elasticmachine
Copy link

elasticmachine commented Jul 24, 2022

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 100.0% (2/2) 💚
Files 50.0% (2/4) 👎 -47.099
Classes 50.0% (2/4) 👎 -47.099
Methods 68.421% (26/38) 👎 -20.911
Lines 68.551% (194/283) 👎 -22.023
Conditionals 100.0% (0/0) 💚

@@ -1,15 +1,18 @@
format_version: 1.0.0
name: apache
title: Apache HTTP Server
version: 1.4.1
version: 1.4.2
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: 1.5 as it isn't a patch.

@@ -1,15 +1,18 @@
format_version: 1.0.0
name: apache
title: Apache HTTP Server
version: 1.4.1
version: 1.4.2
license: basic
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't you want to drop the original license field or is it too early?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that we have to leave them for packages targeting current versions of Kibana. It uses this field from the manifest. Though if it doesn't find it, it defaults to basic.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm.. so we can drop the license field for Kibanas targeting >=8.4.0 or >=8.5.0?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure, it hasn't been implemented yet in kibana.

@mtojek mtojek self-requested a review July 25, 2022 08:55
description: Collect logs and metrics from Apache servers with Elastic Agent.
type: integration
categories:
- web
release: ga
conditions:
kibana.version: "^8.0.0"
elastic.subscription: basic
Copy link
Contributor

Choose a reason for hiding this comment

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

@jsoriano , I understand that for now we are keeping both older license field and subscription.
But when we remove the older field but bring up the integration on older Kibana version won't it be a regression as it won't understand our newer subscription fields ?

Will we be upgrading the kibana version when we remove the older license field ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Current versions of Kibana don't read these values directly from the manifest, but from the registry API. We are adding some compatibility layers in the registry to fill the old fields when only the new ones are available (elastic/package-registry#826). This should help on the migration to the new fields on packages that work with current and older versions of Kibana.
Here we are keeping both fields as a safeguard, as this is going to be the first package using the new fields.
In any case by now it won't do any harm to keep the old license field if wanted.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, all packages have license: basic now, that is the default for Kibana when license is not set.

@jsoriano jsoriano merged commit 1924f8d into elastic:main Jul 28, 2022
@jsoriano jsoriano deleted the apache-license-subscription branch July 28, 2022 16:08
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