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

Enable _dev/deploy for data streams #228

Merged
merged 5 commits into from
Jan 27, 2021

Conversation

mtojek
Copy link
Contributor

@mtojek mtojek commented Jan 20, 2021

Issue: #89

This PR enables custom _dev/deploy for data stream. It will be used by the future AWS service deployer.

@mtojek mtojek self-assigned this Jan 20, 2021
@mtojek mtojek requested a review from ycombinator January 20, 2021 11:51
@elasticmachine
Copy link
Collaborator

elasticmachine commented Jan 20, 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 #228 updated

    • Start Time: 2021-01-27T09:58:39.272+0000
  • Duration: 12 min 31 sec

  • Commit: be253b7

Test stats 🧪

Test Results
Failed 0
Passed 290
Skipped 0
Total 290

@ycombinator
Copy link
Contributor

I think this PR depends on elastic/package-spec#111? I will review this PR once elastic/package-spec#111 is reviewed and merged.

@mtojek
Copy link
Contributor Author

mtojek commented Jan 26, 2021

I updated the package-spec in this branch.

logger.Errorf("can't find _dev/deploy directory")
return nil, ErrNotFound
}
fmt.Println(devDeployPath)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be removed?

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, thanks!

if err == nil {
return packageDevDeployPath, nil
} else if !os.IsNotExist(err) {
return "", errors.Wrapf(err, "stat failed (path: %s)", packageDevDeployPath)
Copy link
Contributor

Choose a reason for hiding this comment

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

Both the errors (the one and the one above for the data stream) are very similar. Can you differentiate them a bit by saying that one failed for the data stream and one for the package?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The path argument would be different, but I extended the error message to include also some context.

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 467f9f5 into elastic:master Jan 27, 2021
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