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

ES yaml should read the inline script #848

Closed
wants to merge 2 commits into from
Closed

ES yaml should read the inline script #848

wants to merge 2 commits into from

Conversation

pavan-godspeed
Copy link
Contributor

@pavan-godspeed pavan-godspeed commented Dec 18, 2023 •
edited

Full description to this PR is described in this issue #794

@pavan-godspeed pavan-godspeed mentioned this pull request Dec 18, 2023
Closed
mastersilv3r
mastersilv3r reviewed Dec 19, 2023
View reviewed changes
src/core/expandVariables.ts
Copy link
Contributor

Choose a reason for hiding this comment

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

Here you should in case of error do process.exit(1), like you did in configLoader. Because module loading has failed, we shouldn't start the service. Rest of this PR is fine. Can you also document in the github issue and link with this PR. Do add in the github issue the problem, the solution, and how to implement sections with necessary details. As well, add a section explaining all the cases which should work, and which should fail. This way we can generate integration test from this github issue as well.

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, I'm on the way..

Copy link

Choose a reason for hiding this comment

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

Need to see the spec/documentation first before checking the PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Documented about this PR in issue, please check it.

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

@mastersilv3r mastersilv3r mastersilv3r left review comments

@siddarthvader siddarthvader Awaiting requested review from siddarthvader

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@pavan-godspeed @mastersilv3r