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

Deprecate testing/environment in favor of elastic-package stack #405

Merged
merged 2 commits into from
Nov 19, 2020
Merged

Deprecate testing/environment in favor of elastic-package stack #405

merged 2 commits into from
Nov 19, 2020

Conversation

mtojek
Copy link
Contributor

@mtojek mtojek commented Nov 19, 2020

What does this PR do?

The PR deprecates the testing/environment setup in favor of the elastic-package stack command.

How to test this PR locally

Follow the new guide for booting up a local stack. Try to modify the integration and reload these changes in Kibana.

Related issues

@mtojek mtojek added enhancement New feature or request deprecation labels Nov 19, 2020
@mtojek mtojek self-assigned this Nov 19, 2020
@elasticmachine
Copy link

elasticmachine commented Nov 19, 2020 •
edited
Loading

💚 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

Expand to view the summary

Build stats

  • Build Cause: [Pull request #405 updated]

  • Start Time: 2020-11-19T16:45:43.924+0000

  • Duration: 10 min 20 sec

Test stats 🧪

Test Results
Failed 0
Passed 10
Skipped 0
Total 10

@mtojek mtojek marked this pull request as ready for review November 19, 2020 12:26
ycombinator
ycombinator reviewed Nov 19, 2020
View reviewed changes
CONTRIBUTING.md Outdated
_Hint_. There is dockerized environment in beats (`cd testing/environments`). Boot it up with the following command:
`docker-compose -f snapshot.yml up --force-recreate`.
~~_Hint_. There is dockerized environment in beats (`cd testing/environments`). Boot it up with the following command:
`docker-compose -f snapshot.yml up --force-recreate`.~~ (deprecated)
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason we should leave this in here, marked as deprecated? I'm wondering if we can just remove this method altogether now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You convinced me to remove the deprecation notice. Fixed.

ycombinator
ycombinator reviewed Nov 19, 2020
View reviewed changes
Copy link
Contributor

@ycombinator ycombinator left a comment

Choose a reason for hiding this comment

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

My main feedback is about the documentation changes.

  • Rather than striking out the old method and marking it as deprecated, I would suggest simply showing the new method of achieving the same thing. I think this is clearer for the user.
  • In many places in the CONTRIBUTING.md you have referred to the cheat sheet for elastic-package. I think this is great but I would also provide the exact replacement command(s) to run with elastic-package. From what I can tell mage Reload was rebuilding all packages and then rebuilding+starting the package registry container, so I would simply provide the equivalent elastic-package commands inline in the CONTRIBUTING.md in place of mage Reload. This will save users an extra hop to the cheat sheet.

ycombinator
ycombinator reviewed Nov 19, 2020
View reviewed changes
CONTRIBUTING.md Outdated
```

The command will boot up a docker cluster with Elasticsearch, Kibana and Package Registry. After every time you
rebuild and reload packages (`mage Reload`), all adjustments in packages will be propagated to the registry.
~~The command will boot up a docker cluster with Elasticsearch, Kibana and Package Registry. After every time you
Copy link
Contributor

Choose a reason for hiding this comment

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

I would remove the strike out presentation and simply rewrite this section with:

  • what does this command do, and
  • what are the elastic-package commands to run for reloading packages

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

ycombinator
ycombinator reviewed Nov 19, 2020
View reviewed changes
CONTRIBUTING.md Outdated
1. When you're developing integrations and you'd like to propagate your changes to the package registry,
use `mage Reload` to rebuild and reload the package registry.

1. ~~When you're developing integrations and you'd like to propagate your changes to the package registry,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's helpful to keep the introductory sentence ("when you're developing..."). Just remove all mentions of mage Reload and replace them with the equivalent elastic-stack commands.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, fixed.

ycombinator
ycombinator reviewed Nov 19, 2020
View reviewed changes
testing/environments/README.md Outdated
Refresh docker images:
Docker Compose files have been deprecated in favor of the `elastic-package stack` command. The tool can be found in:
https://github.com/elastic/elastic-package . With the current building procedure, you will also find the correct binary
in the `build` directory.
Copy link
Contributor

Choose a reason for hiding this comment

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

Personally I don't think this deprecation notice is necessary but I'm okay with keeping it here if you think it's worth it. If we keep it, at what point would you say its okay to remove this altogether?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the deprecation noticed.

@mtojek
Copy link
Contributor Author

mtojek commented Nov 19, 2020

  • Rather than striking out the old method and marking it as deprecated, I would suggest simply showing the new method of achieving the same thing. I think this is clearer for the user.

I've adjusted CONTRIBUTING guide.

  • In many places in the CONTRIBUTING.md you have referred to the cheat sheet for elastic-package. I think this is great but I would also provide the exact replacement command(s) to run with elastic-package. From what I can tell mage Reload was rebuilding all packages and then rebuilding+starting the package registry container, so I would simply provide the equivalent elastic-package commands inline in the CONTRIBUTING.md in place of mage Reload. This will save users an extra hop to the cheat sheet.

Actually, that was goal to ask people to do this extra hop, so we don't have commands written in multiple places. It would make easier maintenance, but I'm fine to keep them in both places :)

ycombinator
ycombinator reviewed Nov 19, 2020
View reviewed changes
testing/environments/README.md

... or with Elastic Agent:
Advanced: if you need to modify the internal Docker compose definition, edit files in `~/.elastic-package/stack`, but
keep in mind that these files shouldn't be modified and your changes will be reverted once you update the `elastic-package`:
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

ycombinator
ycombinator approved these changes Nov 19, 2020
View reviewed changes
Copy link
Contributor

@ycombinator ycombinator left a comment

Choose a reason for hiding this comment

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

LGTM.

@mtojek mtojek merged commit e51a59c into elastic:master Nov 19, 2020
eyalkraft pushed a commit to build-security/integrations that referenced this pull request Mar 30, 2022
@mtojek
Deprecate testing/environment in favor of elastic-package stack (elas…
9099a8f 
…tic#405)

* Deprecate testing/environment in favor of elastic-package stack

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

@ycombinator ycombinator ycombinator approved these changes

Assignees

@mtojek mtojek

Labels
deprecation enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Deprecate and remove testing/environments
3 participants
@mtojek @elasticmachine @ycombinator