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

Default fleet policy removal #1157

Merged
merged 14 commits into from
Mar 29, 2022
Merged

Conversation

lykkin
Copy link
Contributor

@lykkin lykkin commented Feb 22, 2022

What is the problem this PR solves?

Remove references to the default field on the policy; use a default policy id instead.

Checklist

  • 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.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Related issues

Agent changes: elastic/beats#30256

Related discussion:
Closes elastic/beats#29774

@lykkin lykkin added enhancement New feature or request backport-skip Skip notification from the automated backport with mergify Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team v8.2.0 labels Feb 22, 2022
@lykkin lykkin self-assigned this Feb 22, 2022
@lykkin lykkin force-pushed the policy-default-updates branch from 73ee543 to 37dd940 Compare February 22, 2022 05:26
@elasticmachine
Copy link
Contributor

elasticmachine commented Feb 22, 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-03-29T15:24:35.502+0000

  • Duration: 11 min 0 sec

Test stats 🧪

Test Results
Failed 0
Passed 271
Skipped 0
Total 271

🤖 GitHub comments

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

  • /test : Re-trigger the build.

@lykkin lykkin requested a review from blakerouse February 23, 2022 03:52
@lykkin lykkin requested a review from a team March 20, 2022 20:07
@lykkin lykkin force-pushed the policy-default-updates branch from 6fa2686 to 6690340 Compare March 20, 2022 20:22
Copy link
Member

@AndersonQ AndersonQ left a comment

Choose a reason for hiding this comment

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

LGTM, just fix the lining issues. However I cannot access the impact of this change.

@mergify
Copy link
Contributor

mergify bot commented Mar 21, 2022

This pull request is now in conflicts. Could you fix it @lykkin? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b policy-default-updates upstream/policy-default-updates
git merge upstream/main
git push upstream policy-default-updates

@lykkin
Copy link
Contributor Author

lykkin commented Mar 21, 2022

Looks like some of the linting errors are in generated files, so I'm going to track those back to the generator to allow renaming rules of struct fields.

@jlind23 jlind23 requested a review from ph March 23, 2022 15:33
@lykkin lykkin force-pushed the policy-default-updates branch from 427efc2 to 4585280 Compare March 23, 2022 16:48
@lykkin
Copy link
Contributor Author

lykkin commented Mar 23, 2022

/test

@lykkin lykkin force-pushed the policy-default-updates branch from c197f1f to f87c6c1 Compare March 23, 2022 17:46
@lykkin
Copy link
Contributor Author

lykkin commented Mar 23, 2022

finally, it seems the linter is satisfied

@lykkin
Copy link
Contributor Author

lykkin commented Mar 23, 2022

/test

@ph
Copy link
Contributor

ph commented Mar 23, 2022

Looking at this now, thanks for the changer in elastic/go-json-schema-generate#1

Copy link
Contributor

@ph ph left a comment

Choose a reason for hiding this comment

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

Oh linters, so evil :(

@lykkin I am curious where these linters errors were raised in the github action or locally?
Looking at your commits, I believe this one is the required changes b06c48b I would have expected the github action to really complain on a subset.

@ph
Copy link
Contributor

ph commented Mar 23, 2022

@lykkin The code here looks fine to me, I haven't approved yet because I want to make sure the other is approved and I can validate it.

@lykkin
Copy link
Contributor Author

lykkin commented Mar 23, 2022

Oh linters, so evil :(

@lykkin I am curious where these linters errors were raised in the github action or locally? Looking at your commits, I believe this one is the required changes b06c48b I would have expected the github action to really complain on a subset.

The issue was in the generated code needing to be updated to the linter standards. the Id -> ID and Api -> API change on the model required a change in the generated names of all the fields, which propagated to other files. There is still one field (namely model.Agent.Id I believe) that didn't get hit by the transformation on the schema generation, but I think this is a good start.

@lykkin lykkin requested a review from a team March 28, 2022 06:28
@mergify
Copy link
Contributor

mergify bot commented Mar 28, 2022

This pull request is now in conflicts. Could you fix it @lykkin? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b policy-default-updates upstream/policy-default-updates
git merge upstream/main
git push upstream policy-default-updates

@lykkin lykkin merged commit 44a8b57 into elastic:main Mar 29, 2022
Copy link
Contributor

@ph ph left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-skip Skip notification from the automated backport with mergify enhancement New feature or request Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team v8.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Agent] Change logic to find agent policy during Fleet enrollment
5 participants