-
Notifications
You must be signed in to change notification settings - Fork 26
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 existing container apps environment #38
Support existing container apps environment #38
Conversation
@lonegunmanb PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @davidkarlsen for opening this pr! Some review comments, and could you please add a corresponding example to show how to use this new variable along with a corresponding e2e test? Thanks!
@lonegunmanb done, can we let it run to see how the test works? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @davidkarlsen for the update, some review comments but almost LGTM.
@lonegunmanb PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @davidkarlsen and I must apologize for my reckless reviews. I was failed to figure out all issues one time, sorry for that.
I have a new request, we need an extra data source to provide a valid output to module's caller.
b05d83f
to
fabc05a
Compare
@lonegunmanb I've rebased against current main - can we test the e2e's again? |
@davidkarlsen Please wait for #40 and #39 then you're ok to rebase again, thanks! |
Hi @davidkarlsen #39 has been merged so you're good to rebase with the latest main branch and try again, thanks for your patience! |
@lonegunmanb done (PS: The branch os open for collab to merge into) |
Hi @davidkarlsen I believe that the failed e2e test should have been fixed by #42 , would you please rebase your pr with the latest |
@lonegunmanb done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @davidkarlsen , LGTM!
Describe your changes
Support existing app service environment.
Issue number
#37
Checklist before requesting a review
CHANGELOG.md
fileThanks for your cooperation!