app_service - support preview version 21 for java_version
#26304
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
computerlove
force-pushed
the
java-21
branch
4 times, most recently
from
cfe59c8 to
06dceef
Compare
mbfrahry
previously requested changes
Jun 12, 2024
There was a problem hiding this comment.
Hey @computerlove! Thanks for this PR but we've got some failing tests because the new values wasn't added to the correct schema or the tests aren't testing the right resource.
=== RUN TestAccAppService_windowsJava21Java
=== PAUSE TestAccAppService_windowsJava21Java
=== CONT TestAccAppService_windowsJava21Java
testcase.go:113: Step 1/2 error: Error running pre-apply refresh: exit status 1
Error: expected site_config.0.java_version to be one of ["1.7" "1.8" "11"], got 21
with azurerm_app_service.test,
on terraform_plugin_test.tf line 48, in resource "azurerm_app_service" "test":
48: java_version = "21"
--- FAIL: TestAccAppService_windowsJava21Java (11.10s)
=== CONT TestAccAppService_windowsJava17Java
testcase.go:113: Step 1/2 error: Error running pre-apply refresh: exit status 1
Error: expected site_config.0.java_version to be one of ["1.7" "1.8" "11"], got 17
with azurerm_app_service.test,
on terraform_plugin_test.tf line 48, in resource "azurerm_app_service" "test":
48: java_version = "17"
--- FAIL: TestAccAppService_windowsJava17Java (11.67s)
|
@mbfrahry When I update the conflict in CHANGELOG.md, should the change be amended and force pushed, or is a merge commit OK? |
|
@computerlove, we take care of updating the CHANGELOG.md after a PR has been merged, so if there are any changes in there they should be removed. We recommend rebasing your changes on top of main, but merging changes from main into your branch is also acceptable. |
|
Ok, I have rebased and pushed with not changes to CHANGELOG.md. |
stephybun
reviewed
Aug 14, 2024
There was a problem hiding this comment.
Thanks @computerlove! I think we're also missing a test for linux function app?
|
Yeah, looks like it. |
stephybun
previously requested changes
Aug 15, 2024
|
any update? |
|
No. |
|
My interpretation is that, the test is apply the changes and then makes a terraform plan -refresh=false and expects that the plan need no changes. (But why this happens only for java 21, I have no clue) |
erboucherie
reviewed
Nov 15, 2024
jackofallops
requested changes
Nov 28, 2024
There was a problem hiding this comment.
Hi @computerlove - Thanks for this PR. Looking over there's some additional changes needed to accommodate the new version, which I've noted for you below. If you can take a look it should get the test passing, and the fx_version should then be correctly set to JAVA|21-java21 and the tests should pass.
|
Thank you! Will take a look in a few days.
…On Thu, Nov 28, 2024, 14:05 jackofallops ***@***.***> wrote:
***@***.**** requested changes on this pull request.
Hi @computerlove <https://github.com/computerlove> - Thanks for this PR.
Looking over there's some additional changes needed to accommodate the new
version, which I've noted for you below. If you can take a look it should
get the test passing, and the fx_version should then be correctly set to
JAVA|21-java21 and the tests should pass.
------------------------------
In website/docs/r/function_app.html.markdown
<#26304 (comment)>
:
> @@ -279,7 +279,7 @@ The `site_config` block supports the following:
-> **NOTE** User has to explicitly set `ip_restriction` to empty slice (`[]`) to remove it.
-* `java_version` - (Optional) Java version hosted by the function app in Azure. Possible values are `1.8`, `11` & `17` (In-Preview).
+* `java_version` - (Optional) Java version hosted by the function app in Azure. Possible values are `1.8`, `11`, `17`, `21` (In-Preview).
This is the legacy resource, and is not changed by the code changes here,
could you revert this change please?
------------------------------
In website/docs/r/linux_function_app.html.markdown
<#26304 (comment)>
:
> @@ -176,7 +176,7 @@ A `application_stack` block supports the following:
* `use_dotnet_isolated_runtime` - (Optional) Should the DotNet process use an isolated runtime. Defaults to `false`.
-* `java_version` - (Optional) The Version of Java to use. Supported versions include `8`, `11` & `17`.
+* `java_version` - (Optional) The Version of Java to use. Supported versions include `8`, `11`, `17`, `21` (In-Preview).
⬇️ Suggested change
-* `java_version` - (Optional) The Version of Java to use. Supported versions include `8`, `11`, `17`, `21` (In-Preview).
+* `java_version` - (Optional) The Version of Java to use. Supported versions include `8`, `11`, `17`, `21`.
+
+~> **NOTE:** The value `21` is currently in Preview for `java_version`.
------------------------------
In internal/services/appservice/linux_web_app_resource_test.go
<#26304 (comment)>
:
> + ),
+ },
+ data.ImportStep("site_credential.0.password"),
+ })
+}
+
+func TestAccLinuxWebApp_withJre21Java(t *testing.T) {
+ data := acceptance.BuildTestData(t, "azurerm_linux_web_app", "test")
+ r := LinuxWebAppResource{}
+
+ data.ResourceTest(t, r, []acceptance.TestStep{
+ {
+ Config: r.java(data, "21", "JAVA", "21"),
+ Check: acceptance.ComposeTestCheckFunc(
+ check.That(data.ResourceName).ExistsInAzure(r),
+ check.That(data.ResourceName).Key("site_config.0.linux_fx_version").HasValue("JAVA|21-21"),
This is actually due to some missing code to account for this change. Can
you extend the decodeApplicationStackLinux() function to accommodate the
new version 21
here
<https://github.com/hashicorp/terraform-provider-azurerm/blob/c8a5ffba4cbc370d5c89160cbdb4469c89d76b83/internal/services/appservice/helpers/fx_strings.go#L54>
and similarly JavaLinuxFXStringBuilder() here
<https://github.com/hashicorp/terraform-provider-azurerm/blob/c8a5ffba4cbc370d5c89160cbdb4469c89d76b83/internal/services/appservice/helpers/fx_strings.go#L265>
—
Reply to this email directly, view it on GitHub
<#26304 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAF34K2SUNOMGDISA6FOHWD2C4PL7AVCNFSM6AAAAABJF22QWSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDINRYGIYTKMBUGY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***
com>
|
jackofallops
changed the title
app_service - support preview version 21 for java_version
Co-authored-by: jackofallops <11830746+jackofallops@users.noreply.github.com>
|
jackofallops
approved these changes
Dec 6, 2024
jackofallops
dismissed stale reviews from katbyte, stephybun, and mbfrahry
stale
|
GHA Unit Test failure due to unrelated test code on merge comparison branch that has been fixed in |
jackofallops
added a commit
that referenced
this pull request
Dec 6, 2024
jackofallops
added a commit
that referenced
this pull request
Dec 13, 2024
* Update CHANGELOG.md for #26304 * Update CHANGELOG.md for #28211 * Update for #28016 * Update for #28139 * Update for #27776 * Update for #28227 * Update for #28080 * Update for #28228 * Update for #27915 * reword nginx api upgrade * Update for #28160 * Update for #28043 * run changelog-update-for-release.sh --------- Co-authored-by: stephybun <steph@hashicorp.com> Co-authored-by: kt <kt@katbyte.me>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Community Note
Description
Add Java 21 as a allowed value for webapp services.
PR Checklist
Changes to existing Resource / Data Source
Testing
Change Log
Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.
azurerm_linux_web_app- Support Java 21. (#25490)azurerm_windows_web_app- Support Java 21. (#25490)azurerm_linux_function_app- Support Java 21. (#24754)azurerm_windows_function_app- Support Java 21. (#24754)This is a (please select all that apply):
Related Issue(s)
fixes #25490
fixes #24754