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

As a developer, I want changes I make to the front end to be visible when I deploy from Netbeans #6953

Closed
pdurbin opened this issue Jun 1, 2020 · 11 comments · Fixed by #7695

Comments

@pdurbin
Copy link
Member

pdurbin commented Jun 1, 2020

[Formerly known as As a developer, I want changes I make to the front end to be visible when I deploy from Netbeans]

Ever since 5f99037 developers have had to remember to comment out part of src/main/webapp/WEB-INF/web.xml in order for changes they are making to the front end to be visible when deploying from Netbeans. At the very least we should document this in the dev guide. This is the part that has to be commented out (but not committed, of course):

<context-param>
    <param-name>javax.faces.FACELETS_REFRESH_PERIOD</param-name>
    <param-value>-1</param-value>
</context-param>

Even better would be to somehow fix it so developers don't have to comment this out but I'd be satisfied with some docs for now.


[GPD - 2020-12-16 - edited the original description -

Original said to also comment out

<context-param>
    <param-name>javax.faces.FACELETS_BUFFER_SIZE</param-name>
    <param-value>102400</param-value>
</context-param>

but doing so causes flash messages to become "sticky" - see issue #7305]

@pdurbin
Copy link
Member Author

pdurbin commented Jul 2, 2020

Here's a workaround to avoid accidentally checking in a change to web.xml.

  • Edit src/main/webapp/WEB-INF/web.xml and comment out the lines above.
  • Mark the file as not changed with git update-index --assume-unchanged src/main/webapp/WEB-INF/web.xml
  • Confirm that git doesn't think the file has been edited with git status and git ls-files -v | grep "^[a-z]" (which lists files that have been marked as "assume unchanged")

If you actually need to make an edit to web.xml, you must put it back to normal with this:

  • git update-index --no-assume-unchanged src/main/webapp/WEB-INF/web.xml

References:

@poikilotherm
Copy link
Contributor

As @pdurbin already mentioned in #7457, using MicroProfile Config API would be an easy way to completely mitigate this problem without recompiling and just changing a config setting.

When MicroProfile Config API 2.0 gets released, it's even easier by switching profiles and providing different setting inside those accordingly. See eclipse/microprofile-config#418

Problem: this is not yet supported with Payara, <context-param> does not support variable substitution from web.xml.
It works for @WebInitParam in @WebServlet annotations. We would need to create an issue with the Payara people to use TranslatedConfigView.expandValue() inside org.glassfish.web.deployment.node.InitParamNode.

@mheppler mheppler changed the title As a developer, I want changes I make to the front end to be visible when I deploy from Netbeans NetBeans deploy vs web.xml config - Increased buffer size breaks auto deploy on save (requires developer config workaround) Jan 27, 2021
@mheppler mheppler changed the title NetBeans deploy vs web.xml config - Increased buffer size breaks auto deploy on save (requires developer config workaround) NetBeans deploy vs web.xml config - Increased buffer size breaks auto display on save (requires developer config workaround) Jan 28, 2021
poikilotherm added a commit to poikilotherm/dataverse that referenced this issue Feb 1, 2021
@djbrooke djbrooke assigned pdurbin and unassigned pdurbin Mar 10, 2021
@pdurbin
Copy link
Member Author

pdurbin commented Mar 10, 2021

@poikilotherm we talked about this issue in sprint planning today and I just noticed your "Test using MPCONFIG variable substitution in web.xml" commit above. Does this actually work?!? I'd love a fix for this and if MPCONFIG can come to the rescue, that would be just fine with me!

Also, please note that the original description of this issue has been updated. We figured out we only need to comment out FACELETS_REFRESH_PERIOD.

@poikilotherm
Copy link
Contributor

poikilotherm commented Mar 10, 2021

Nope. Like I wrote above, for context params variable substitution is not supported in Payara. We might open an enhancement request and vote vote vote, especially for stuff like a configurable context root etc.

@poikilotherm
Copy link
Contributor

@pdurbin as requested, I opened payara/Payara#5157. Remember, we will need to vote vote vote vote vote vote to make this happen. So spread the word...

@pdurbin
Copy link
Member Author

pdurbin commented Mar 11, 2021

@poikilotherm thanks!

Meanwhile, @scolapasta found what looks like a good fix.

Here's the change we need to make:

diff --git a/src/main/webapp/WEB-INF/web.xml b/src/main/webapp/WEB-INF/web.xml
index 9ccec3d9d..436f9a505 100644
--- a/src/main/webapp/WEB-INF/web.xml
+++ b/src/main/webapp/WEB-INF/web.xml
@@ -54,7 +54,7 @@
     </context-param>
     <context-param>
         <param-name>javax.faces.FACELETS_REFRESH_PERIOD</param-name>
-        <param-value>-1</param-value>
+        <param-value>${dataverse.facelets.refresh.period}</param-value>
     </context-param>
     <!-- JSF mapping -->
     <filter>
(venv) beamish:dataverse pdurbin$ 

Then, in domain.xml, developers would put this (positive 1):

<jvm-options>-Ddataverse.facelets.refresh.period=1</jvm-options>

Production installations would put this (negative 1):

<jvm-options>-Ddataverse.facelets.refresh.period=-1</jvm-options>

For more on these numbers from https://javadoc.io/static/jakarta.faces/jakarta.faces-api/2.3.2/javax/faces/application/ViewHandler.html#FACELETS_REFRESH_PERIOD_PARAM_NAME

When a page is requested, what interval in seconds should the compiler check for changes. If you don't want the compiler to check for changes once the page is compiled, then use a value of -1. Setting a low refresh period helps during development to be able to edit pages in a running application.The runtime must also consider the facelets.REFRESH_PERIOD param name as an alias to this param name for backwards compatibility with existing facelets tag libraries. If ProjectStage is set to Production and this value is not otherwise specified, the runtime must act as if it is set to -1.

@poikilotherm
Copy link
Contributor

poikilotherm commented Mar 16, 2021

@pdurbin I gave this a shot today in my reproducer project with Arquillian.

🥳 Good news: WE CAN USE ${MPCONFIG=key} HERE 🥳

(And since 5.2021.1 we can also give a default value via ${MPCONFIG=key:default})

I could not test so far if we can update this setting dynamically. Most likely you will need to redeploy, as the context is initialized once and an update would need a programmatic call to change to context. (If we want this, we might take a look at manipulating the context from the Dataverse codebase...)

Note: using a variable here makes this not very portable, as var substitution is not part of the Jakarta EE standard and violates the TCK checks. But I don't think we care 😸

@pdurbin
Copy link
Member Author

pdurbin commented Mar 16, 2021

@poikilotherm that's awesome. I don't think it needs to be set dynamically. Would you want to make a pull request? (I suppose this includes an upgrade to Payara 5.2021.1.) Or should we go with the JVM approach (which I'm comfortable doing.)

@pdurbin
Copy link
Member Author

pdurbin commented Mar 24, 2021

@poikilotherm I brought this up in sprint planning today and there is little desire to add a JVM option. Pull request #7695 looks great but requires an update to Payara. Did you say you might be able to make a similar pull request that doesn't require this upgrade? Thanks.

@poikilotherm
Copy link
Contributor

Aye. PRs forthcoming.

poikilotherm added a commit to poikilotherm/dataverse that referenced this issue Mar 25, 2021
poikilotherm added a commit to poikilotherm/dataverse that referenced this issue Mar 25, 2021
poikilotherm added a commit to poikilotherm/dataverse that referenced this issue Mar 26, 2021
…yara 5.2020.6 IQSS#6953

Reverts commit ca35411

Due to the nature of the application server, when the web app context is
initialised, it cannot yet access the deployments default MPCONFIG
source within META-INF/microprofile-config.properties.

As a consequence, as long as we do not update to Payara 5.2021.1, we
cannot provide sane defaults within the variable. That means that every
installation would need to set the variables in MPCONFIG sources
available at startup time and before the deployment finished: any
are possible that are baked into Payara.

As this doesn't seem feasible, this PR will be blocked until IQSS#7700 is
resolved.
@poikilotherm
Copy link
Contributor

poikilotherm commented Mar 26, 2021

While it is possible to use the variable substitution, we cannot apply defaults from within our apps codebase. I put an extensive explanation in 48d6501 (tested this in deployments to verify).

This is also true for all the other places we use the variable substitution in. Payara resolved this with payara/Payara#5089, included in 5.2021.1. We will need to either wait until we go for 5.2021.1+ in #7700 / #7701 or tell everyone to set the vars 😄 (I'm sure what the answer is...)

Sorry @pdurbin. Guess we should either bribe @scolapasta or wait...

@pdurbin pdurbin changed the title NetBeans deploy vs web.xml config - Increased buffer size breaks auto display on save (requires developer config workaround) As a developer, I want changes I make to the front end to be visible when I deploy from Netbeans Apr 5, 2021
pdurbin added a commit to poikilotherm/dataverse that referenced this issue Aug 17, 2021
pdurbin added a commit to poikilotherm/dataverse that referenced this issue Aug 17, 2021
pdurbin added a commit to poikilotherm/dataverse that referenced this issue Aug 17, 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 a pull request may close this issue.

2 participants