-
Notifications
You must be signed in to change notification settings - Fork 459
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
[Kubenretes Integration] Adding deployment condition status in kube state deployment dataset #6821
Conversation
packages/kubernetes/manifest.yml
Outdated
@@ -11,7 +11,7 @@ categories: | |||
- kubernetes | |||
release: ga | |||
conditions: | |||
kibana.version: "^8.8.0" | |||
kibana.version: "^8.8.2" |
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.
are you planning to backport elastic/beats#35999 to this version? I believe 8.8.2
was already released, so it will not land in 8.8.2, isn't it?
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.
Deliberately made because this field will be present only when elastic/beats#35999 is merged.
And yes we will need a version later than 8.8.2. Is not that correct?
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.
We don't need to update the kibana version as we are not using any new kibana feature for this. The new fields will start get populated in version beats 8.10. But that doesn't mean we need to set it to 8.10 in the kibana version constraints. Until the agent 8.10 is released the added fields will not be populated. If we set the version to 8.10 here, then we won't be able to introduce new immediate changes to the integration.
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.
Makes sense! Thanks for clarifications @MichaelKatsoulis .
Reverted change for kibana. Thank you both
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.
It's more correct if we set this to 8.10
. Otherwise the new package's version will mention in the docs the new field but this will not be populated right? I believe fields' docs should be aligned with the features.
Not having the option to do changes for previous versions is a known issue but we should not deliver inaccurate packages just to by pass this.
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.
So you are suggesting to freeze this PR until 8.10 gets released or until we need to update the kibana constraint for a different PR ? Because just putting 8.10 now prevents us from any other update targeting to 8.9 or 8.8
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.
Freezing it won't be a real problem as long as the feature is not yet released, right?
So I'm suggesting to either freeze it or/and raise this conversation to a wider audience to make sure that we have a solid process to handle cases like these in the future.
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.
Yes let's freeze this until it makes sense.
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.
So by reading the above discussion:
- We will keep the kibana constrain to "^8.8.0"
- Wait until 8.10 is released and then merge this integration
Raise a question in today's meeting call: "how we handle integrations upgrades that need to match specific beats version?"
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.
You will also need to update the constrain.
So it's: Wait until 8.10
is released and then merge this integration with 8.10.0
constrain.
🌐 Coverage report
|
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.
Since we agree on this let's stall the PR for now.
kibana.version
should be ^8.10.0
for this be merged.
Hi! We just realized that we haven't looked into this PR in a while. We're sorry! We're labeling this issue as |
Hi! This PR has been stale for a while and we're going to close it as part of our cleanup procedure. We appreciate your contribution and would like to apologize if we have not been able to review it, due to the current heavy load of the team. Feel free to re-open this PR if you think it should stay open and is worth rebasing. Thank you for your contribution! |
Reopening and updated constrain to ^8.10.0 as per discusion above. |
9aa4df4
to
b64cd98
Compare
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.
LGTM
Package kubernetes - 1.44.0 containing this change is available at https://epr.elastic.co/search?package=kubernetes |
**** DONT MERGE UNTIL 8.10 is released****
What does this PR do?
Introduces in Kubernetes Integration the new deployment condition status field
Checklist
changelog.yml
file.How to test this PR locally
See inforamtion here: elastic/elastic-agent#2995
Related issues