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

adding docs for default container in 8.9 #386

Merged
merged 1 commit into from
Aug 4, 2023
Merged

adding docs for default container in 8.9 #386

merged 1 commit into from
Aug 4, 2023

Conversation

gizas
Copy link
Contributor

@gizas gizas commented Aug 3, 2023

@ChrsMark
Copy link
Member

ChrsMark commented Aug 3, 2023

@gizas I cannot follow which PRs should be back-ported and how all these are related. Can you make it more clear in the description? We will need this as future reference as well.

For example how is this one related with #384 and why we have this warning message at #380 (comment)?

@ChrsMark
Copy link
Member

ChrsMark commented Aug 3, 2023

Also please add the original PR that this one backports.

#380 that is mentioned in the description changes only 2 lines but this one 119. How is this possible? Do we miss sth here?

Copy link
Member

@ChrsMark ChrsMark left a comment

Choose a reason for hiding this comment

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

Let's clarify #386 (comment) before merging.

@gizas
Copy link
Contributor Author

gizas commented Aug 3, 2023

Initial PR that was merged to master was #304

This one needs to be backported to 8.9.

The warning comes from the automated backport of 2baca48 which failed

@ChrsMark
Copy link
Member

ChrsMark commented Aug 3, 2023

Why #380 is mentioned in the description then? If that's wrong can you please fix it?

@gizas
Copy link
Contributor Author

gizas commented Aug 3, 2023

Why #380 is mentioned in the description then? If that's wrong can you please fix it?

It is not wrong, the above PR might have been merged (as main included forward changes than 8.9) but the automatic backport failed.

That is why I have added the link to indicate that the manual backport is needed

@ChrsMark
Copy link
Member

ChrsMark commented Aug 3, 2023

Well #380 is not a backport, it's against main, right?

Could you update the description of this current PR to clearly mention why it is related to any other PRs? The See relevant backport that failed #3801 is quite confusing since without context makes me think that for some reason is related to this one somehow.

@gizas
Copy link
Contributor Author

gizas commented Aug 3, 2023

Ok I updated the description.

FYI the #380 is against main but it had the label backport-8.9, so this would trigger the backport. This is probably what it was not clearly from the beginning.

I have closed the duplicate #386

@ChrsMark
Copy link
Member

ChrsMark commented Aug 3, 2023

Thanks!

FYI an PR against main cannot be called a "backport" no matter what labels it has. PRs against main are considered as original PRs and then all their "mirrored" PRs against specific tags/version-branches are their backports. That's the semantic that we have been following.

@@ -92,7 +92,7 @@ https://github.com/elastic/elastic-agent/tree/main/deploy/kubernetes/elastic-age
[discrete]
== Configure hints autodiscovery

To enable hints, you must add `hints.enabled: true` to the provider's configuration:
To enable hints autodiscovery, you must add `hints.enabled: true` to the provider's configuration:
Copy link
Member

@ChrsMark ChrsMark Aug 3, 2023

Choose a reason for hiding this comment

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

That section does not exist in https://github.com/elastic/ingest-docs/pull/304/files. Is this coming from another PR? If so we should mention it as well in the description.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the desc, it was the #380

@ChrsMark ChrsMark self-requested a review August 3, 2023 12:47
Copy link
Contributor

@kilfoyle kilfoyle left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀
Thanks for tidying up the description @gizas

Copy link
Member

@ChrsMark ChrsMark left a comment

Choose a reason for hiding this comment

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

LGTM

[source,yaml]
----
annotations:
co.elastic.hints/package: "container_logs"
Copy link
Member

Choose a reason for hiding this comment

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

Just a note that container_logs package only made it to main with elastic/elastic-agent#2981 but it doesn't make sense to restrict it since we always pull the templates from latest main: https://github.com/elastic/elastic-agent/blob/083bf7805afc9d57727b099da6fa48cf96719585/deploy/kubernetes/elastic-agent-standalone-kubernetes.yaml#L686

@gizas gizas merged commit 0f27e6e into 8.9 Aug 4, 2023
@gizas gizas deleted the backportinghints89 branch August 4, 2023 08:32
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.

3 participants