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

refactor!: Remove deployment conf field from VaadinSession #18319

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

refactor!: Remove deployment conf field from VaadinSession #18319

wants to merge 8 commits into from

Conversation

tepi
Copy link
Contributor

@tepi tepi commented Dec 15, 2023

Fixes #18318

@tepi tepi marked this pull request as draft December 15, 2023 13:25
@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 15, 2023 •
edited
Loading

Test Results

1 045 files  ±  0  1 045 suites  ±0   1h 14m 19s ⏱️ + 3m 32s
6 736 tests ±  0  6 691 ✔️ +  6  45 💤 ±0  0 ❌  - 1 
7 036 runs  +28  6 980 ✔️ +34  56 💤 ±0  0 ❌  - 1 

Results for commit c107a81. ± Comparison against base commit 757c8cb.

♻️ This comment has been updated with latest results.

mcollovati
mcollovati requested changes Dec 19, 2023
View reviewed changes
Copy link
Collaborator

@mcollovati mcollovati left a comment

Choose a reason for hiding this comment

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

The change looks good and makes sense.
A minor drawback will be that it might break some test configurations on other repositories. In MPR for example, it is used in a couple of tests, and the problem here may be that we use the same MPR codebase against different Flow version.

Potentially also user projects may be affected, since setConfiguration() is a public API, even though I suppose VaadinSession.setConfiguration() is not widely used.

flow-server/src/main/java/com/vaadin/flow/server/VaadinSession.java Outdated Show resolved Hide resolved
flow-server/src/test/java/com/vaadin/flow/component/InvalidUrlTest.java Show resolved Hide resolved
flow-server/src/main/java/com/vaadin/flow/server/VaadinSession.java Outdated Show resolved Hide resolved
@tepi tepi marked this pull request as ready for review December 19, 2023 10:24
@mcollovati
Copy link
Collaborator

Some tests to fix, but other than that LGTM

@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Dec 19, 2023

Quality Gate Passed Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

3 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@mcollovati mcollovati changed the title #18318 Remove deployment conf field from VaadinSession refactor!: Remove deployment conf field from VaadinSession Dec 19, 2023
@tepi
Copy link
Contributor Author

tepi commented Dec 19, 2023

Despite approval, do not merge. This should be postponed until major release of Flow (25), since this changes public API.

@mcollovati
Copy link
Collaborator

Would it be better to make it a draft again, to prevent unwanted merge?

@tepi tepi marked this pull request as draft December 19, 2023 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcollovati mcollovati mcollovati approved these changes

Assignees
No one assigned
Labels
planned-in-25.0 +1.0.0
Projects
Vaadin Flow ongoing work (Vaadin 10+)
Status: 🔎Iteration reviews
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

DeploymentConfiguration is retained in VaadinSession over session store-load cycle
3 participants
@tepi @mcollovati @vaadin-bot