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

Change references to ="$(expression)" from =$(expression) in scripts #713

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

aplybeah
Copy link
Contributor

Ticket

Resolves #669

Changes

Context for reviewers

This is related to the changes made in #661

Testing

N/A

@aplybeah aplybeah marked this pull request as ready for review August 2, 2024 20:08
bin/run-command Outdated
@@ -88,18 +88,18 @@ overrides=$(cat << EOF
]
}
EOF
)
)"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure about this one? It might because the exception since there's an EOF involved

<!-- begin PR environment info -->
## Preview environment
- Service endpoint: ${service_endpoint}
- Deployed commit: ${image_tag}
<!-- end PR environment info -->
EOF
)
)"
Copy link
Contributor

Choose a reason for hiding this comment

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

Same opinion as above about the EOF

Copy link
Contributor

@coilysiren coilysiren left a comment

Choose a reason for hiding this comment

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

This LGTM, but I want @lorenyu to chime in about whether or not we should retest all these scripts. That seems like a like of work for a change of this small size.

@lorenyu
Copy link
Contributor

lorenyu commented Aug 16, 2024

@aplybeah sorry I totally missed this PR, please ping me again if you don't get a response, don't let PRs just sit around as the PR author you're ultimately responsible for driving things to completion.

Please follow the template development workflow and create a platform-test PR with these changes so that we can see that these changes all pass in CI, and post a link to that PR in this one.

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.

Change references to ="$(expression)" from =$(expression) in scripts
3 participants