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

Support kubernetes service deployer #130

Merged

Conversation

mtojek
Copy link
Contributor

@mtojek mtojek commented Feb 8, 2021

What does this PR do?

This PR defines files used by the Kubernetes service deployer.

Why is it important?

These files will define a custom service deployed on top of the Kubernetes cluster. The Elastic Agent can monitor this service (e.g. nginx deployed in the cluster). If there aren't any files required to be installed in the cluster, it's mandatory to set the .empty file as a mark for the test runner to use Kubernetes service deployer (empty directory _dev/deploy/k8s won't be versioned).

Checklist

Related issues

@mtojek mtojek self-assigned this Feb 8, 2021
@elasticmachine
Copy link

elasticmachine commented Feb 8, 2021

💚 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 #130 updated

  • Start Time: 2021-02-10T09:18:49.868+0000

  • Duration: 3 min 22 sec

  • Commit: 71c0e67

Trends 🧪

Image of Build Times

@mtojek mtojek requested a review from ycombinator February 9, 2021 10:59
@mtojek mtojek marked this pull request as ready for review February 9, 2021 11:00
@mtojek
Copy link
Contributor Author

mtojek commented Feb 9, 2021

@ycombinator is ready for review, but please take a look at the elastic/elastic-package#246 to verify if the proposal here is correct.

@@ -17,8 +17,8 @@ func loadItemContent(itemPath, mediaType string) ([]byte, error) {
return nil, errors.Wrap(err, "reading item file failed")
}

if len(itemData) == 0 {
return nil, errors.New("file is empty")
if len(itemData) == 0 && mediaType != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

It took me a second to realize that this additional check was required to properly handle the .empty file case. In the case of the .empty file, len(itemData) == 0 but mediaType == "" so this check will evaluate to false, which is what we want. Without the additional mediaType != "" clause, this check would have evaluated to true and returned an error, which is not what we want.

Would you mind adding a comment above this check explaining why we need to check both the length of itemData and the media type?

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.

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'll merge it once the CI is green and doesn't complain.

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.

Left a minor comment about adding an explanatory comment. 🙂

Otherwise, LGTM.

@mtojek mtojek merged commit cd2c4d7 into elastic:master Feb 10, 2021
rw-access pushed a commit to rw-access/package-spec that referenced this pull request Mar 23, 2021
* Refactor: type visibility, comments

* Adjust structures

* Use ucfg to support flat and extended manifest files

* Fix: make check

* Fix: make gomod

* Bring back VarValue

* Fix: use ucfg in system test runner

* Fix: use config instead of yaml

* Try: implement Unpacker not ConfigUnpacker
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