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.

Sign up for GitHub

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

Jump to bottom

feat(elastic-agent-services): add another tag to docker image for agentless use #6874

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

feat(elastic-agent-services): add another tag to docker image for agentless use #6874

wants to merge 2 commits into from
+83 −13

Conversation

olegsu
Copy link

@olegsu olegsu commented Feb 14, 2025

What does this PR do?

This pr will add additional tag to docker image elastic-agent-service in form of elastic-agent-service:git-{SHORT_COMMIT}

Why is it important?

Elastic-Agent is used in Agentless in both ECH and ES3.
While in ECH we must a version that will match the ELK stack, in ES3 we dont have that restriction.
Two reasons why this is important for ES3:

  1. Automatic release flow
  2. Continuous feature release

I do not want to elaborate on the details in a public repo, please check the RFC and https://github.com/elastic/security-team/issues/9273 with full context

Checklist

  • I have read and understood the pull request guidelines of this project.
  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in ./changelog/fragments using the changelog tool
  • I have added an integration test or an E2E test

Disruptive User Impact

How to test this PR locally

Related issues

Questions to ask yourself

  • How are we going to support this in production?
  • How are we going to measure its adoption?
  • How are we going to debug this?
  • What are the metrics I should take care of?
  • ...

@olegsu olegsu requested a review from a team as a code owner February 14, 2025 21:52
@mergify mergify bot assigned olegsu Feb 14, 2025
@mergify Mergify
Copy link
Contributor

mergify bot commented Feb 14, 2025

This pull request does not have a backport label. Could you fix it @olegsu? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-./d./d is the label that automatically backports to the 8./d branch. /d is the digit
  • backport-active-all is the label that automatically backports to all active branches.
  • backport-active-8 is the label that automatically backports to all active minor branches for the 8 major.
  • backport-active-9 is the label that automatically backports to all active minor branches for the 9 major.

@olegsu olegsu force-pushed the support-additonal-docker-tags branch 2 times, most recently from e60e748 to 3186eb4 Compare February 15, 2025 15:01
@olegsu olegsu added the backport-9.0 Automated backport to the 9.0 branch label Feb 16, 2025
michalpristas
michalpristas reviewed Feb 17, 2025
View reviewed changes
dev-tools/mage/dockerbuilder.go Outdated
// additional tags should not be created with
for _, tag := range additionalTags {
if err := b.dockerSave(tag, "{{.Name}}{{if .OS}}-{{.OS}}{{end}}{{if .Arch}}-{{.Arch}}{{end}}"); err != nil {
return fmt.Errorf("failed to save docker as artifact: %w", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

specify a tag for narrowing down specifics in case of failure

michalpristas
michalpristas approved these changes Feb 17, 2025
View reviewed changes
Copy link
Contributor

@michalpristas michalpristas left a comment

Choose a reason for hiding this comment

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

i think it looks ok.

@michalpristas michalpristas self-requested a review February 17, 2025 10:10
michalpristas
michalpristas reviewed Feb 17, 2025
View reviewed changes
dev-tools/mage/dockerbuilder.go Outdated
Comment on lines 199 to 207
moreTags := []string{}
if commit, found := b.ExtraVars["commit"]; found && b.DockerVariant == Service {
if len(commit) >= 12 {
moreTags = append(moreTags, fmt.Sprintf("%s:%s", b.imageName, "git-"+commit[:12]))
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

ExtraVars contains raw values, expanded values are inside evalContext, use that one and replace also one for repository above

Suggested change
moreTags := []string{}
if commit, found := b.ExtraVars["commit"]; found && b.DockerVariant == Service {
if len(commit) >= 12 {
moreTags = append(moreTags, fmt.Sprintf("%s:%s", b.imageName, "git-"+commit[:12]))
}
}
moreTags := []string{}
if commitI, found := b.evalContext["commit"]; found && b.DockerVariant == Service {
commit, ok := commitI.(string)
if ok && len(commit) >= 12 {
moreTags = append(moreTags, fmt.Sprintf("%s:git-%s", b.imageName, commit[:12]))
}
}

pkoutsovasilis
pkoutsovasilis reviewed Feb 17, 2025
View reviewed changes
Copy link
Contributor

@pkoutsovasilis pkoutsovasilis left a comment •
edited
Loading

Choose a reason for hiding this comment

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

hey @olegsu 👋 thanks for opening this PR. I believe that already @michalpristas has left some comments that capture some issues with this PR (ty @michalpristas ). However, I kinda have another thinking that affects the approach of this PR. I am not a fan of implicit actions, and here the new tag you try to introduce happens ambiguously if commit is found in extra vars. However, IMO, adding an extra vars shouldn't affect the tags as the two are not profoundly coupled. That said, would it make sense to you as well to alternate this PR, promoting explicitness, e.g. introduce extraTags in package.yml and do utilise them from there? in this way if in the future we need to change/remove/add another tag this will be really straight-forward

@pierrehilbert pierrehilbert added the Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team label Feb 18, 2025
@elasticmachine
Copy link
Contributor

Pinging @elastic/elastic-agent-control-plane (Team:Elastic-Agent-Control-Plane)

@olegsu olegsu force-pushed the support-additonal-docker-tags branch from 3186eb4 to 285c705 Compare February 18, 2025 14:55
@olegsu
Copy link
Author

olegsu commented Feb 18, 2025

hey @olegsu 👋 thanks for opening this PR. I believe that already @michalpristas has left some comments that capture some issues with this PR (ty @michalpristas ). However, I kinda have another thinking that affects the approach of this PR. I am not a fan of implicit actions, and here the new tag you try to introduce happens ambiguously if commit is found in extra vars. However, IMO, adding an extra vars shouldn't affect the tags as the two are not profoundly coupled. That said, would it make sense to you as well to alternate this PR, promoting explicitness, e.g. introduce extraTags in package.yml and do utilise them from there? in this way if in the future we need to change/remove/add another tag this will be really straight-forward

Thank you for the feedback!
It makes sense, let me update the pr

@olegsu olegsu force-pushed the support-additonal-docker-tags branch 4 times, most recently from a22f570 to d67f9e1 Compare February 18, 2025 17:21
@blakerouse
Copy link
Contributor

Please wait for approval from @pkoutsovasilis before merging, but this looks good to me with the latest updates.

@olegsu olegsu force-pushed the support-additonal-docker-tags branch from 8f77509 to 88d1289 Compare February 19, 2025 00:20
@jlind23
Copy link
Contributor

jlind23 commented Feb 19, 2025

@blakerouse Thanks for the approval but I am a bit confused. Didn't we agreed on giving approval only once CI is green to avoid code changes to make CI happy after approval that would not dismiss your review?

@olegsu olegsu force-pushed the support-additonal-docker-tags branch from 88d1289 to 5a12ef9 Compare February 24, 2025 17:01
@olegsu olegsu force-pushed the support-additonal-docker-tags branch 7 times, most recently from c4eee0c to a2c4397 Compare February 25, 2025 21:25
pkoutsovasilis
pkoutsovasilis approved these changes Feb 26, 2025
View reviewed changes
Copy link
Contributor

@pkoutsovasilis pkoutsovasilis left a comment

Choose a reason for hiding this comment

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

Thanks for going through my comments @olegsu this LGTM. I had a look in the CI failures and they all seem to be related to already reported flaky tests and a new one which I have raised with eng prod team. That said, I invoked retries to get this PR merged 🙂

@olegsu
feat(elastic-agent-services): add another tag to docker image for age…
9be5032 
…ntless use

p

override the name in the template

use map copy

remove fmt

update comments

typo
@olegsu
(ci:fix) linting error on G115
6c9b1f3
@olegsu olegsu force-pushed the support-additonal-docker-tags branch from a2c4397 to 6c9b1f3 Compare February 26, 2025 12:26
@elasticmachine
Copy link
Contributor

elasticmachine commented Feb 26, 2025 •
edited
Loading

⏳ Build in-progress, with failures

Failed CI Steps

History

cc @olegsu

@elastic-sonarqube Elastic SonarQube
Copy link

Quality Gate passed Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarQube

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michalpristas michalpristas michalpristas approved these changes

@blakerouse blakerouse blakerouse approved these changes

@pkoutsovasilis pkoutsovasilis pkoutsovasilis approved these changes

Assignees

@olegsu olegsu

Labels
backport-9.0 Automated backport to the 9.0 branch skip-changelog Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@olegsu @elasticmachine @blakerouse @jlind23 @michalpristas @pkoutsovasilis @pierrehilbert