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

chore: Upgraded Snake YAML version to 2.0 #23572

Merged
merged 9 commits into from Jun 8, 2023
Merged

Conversation

nidhi-nair
Copy link
Contributor

@nidhi-nair nidhi-nair commented May 22, 2023

Description

Upgrades SnakeYaml dependency version forcefully to 2.0 to overcome this issue, as advised here.

This version tag can be reverted when we upgrade to Spring 6.1, which is when the library aims to upgrade the version themselves.

Fixes https://github.com/appsmithorg/appsmith-ee/issues/1233

Type of change

  • Chore (housekeeping or task changes that don't impact user perception)

Testing

This PR will be tested during regression.

@nidhi-nair nidhi-nair added the Backend This marks the issue or pull request to reference server code label May 22, 2023
@nidhi-nair nidhi-nair requested a review from sharat87 May 22, 2023 03:51
@nidhi-nair nidhi-nair self-assigned this May 22, 2023
@github-actions github-actions bot added the skip-changelog Adding this label to a PR prevents it from being listed in the changelog label May 22, 2023
sharat87
sharat87 previously approved these changes May 23, 2023
@nidhi-nair
Copy link
Contributor Author

/ok-to-test

@github-actions
Copy link

Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/5053512323.
Workflow: Appsmith External Integration Test Workflow.
Commit: ``.
PR: 23572.
Perf tests will be available at https://app.appsmith.com/app/performance-infra-dashboard/pr-details-638dd7cd2913ba43778b915e?pr=23572&runId=5053512323_1

@sharat87
Copy link
Member

/ok-to-test

@github-actions
Copy link

Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/5131499636.
Workflow: Appsmith External Integration Test Workflow.
Commit: ``.
PR: 23572.
Perf tests will be available at https://app.appsmith.com/app/performance-infra-dashboard/pr-details-638dd7cd2913ba43778b915e?pr=23572&runId=5131499636_1

@sharat87
Copy link
Member

sharat87 commented Jun 2, 2023

@mohanarpit @nidhi-nair, still failing from ff4j:

Caused by: java.lang.NoSuchMethodError: org.yaml.snakeyaml.constructor.SafeConstructor: method 'void <init>()' not found
	at org.ff4j.parser.yaml.YamlParser.<init>(YamlParser.java:55) ~[ff4j-config-yaml-2.0.0.jar:2.0.0]
	at com.appsmith.server.configurations.FeatureFlagConfig.ff4j(FeatureFlagConfig.java:13) ~[classes/:na]
	at com.appsmith.server.configurations.FeatureFlagConfig$$SpringCGLIB$$0.CGLIB$ff4j$0(<generated>) ~[classes/:na]
	at com.appsmith.server.configurations.FeatureFlagConfig$$SpringCGLIB$$2.invoke(<generated>) ~[classes/:na]
	at org.springframework.cglib.proxy.MethodProxy.invokeSuper(MethodProxy.java:258) ~[spring-core-6.0.8.jar:6.0.8]
	at org.springframework.context.annotation.ConfigurationClassEnhancer$BeanMethodInterceptor.intercept(ConfigurationClassEnhancer.java:331) ~[spring-context-6.0.8.jar:6.0.8]
	at com.appsmith.server.configurations.FeatureFlagConfig$$SpringCGLIB$$0.ff4j(<generated>) ~[classes/:na]
	at jdk.internal.reflect.GeneratedMethodAccessor290.invoke(Unknown Source) ~[na:na]
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) ~[na:na]
	at java.base/java.lang.reflect.Method.invoke(Method.java:568) ~[na:na]
	at org.springframework.beans.factory.support.SimpleInstantiationStrategy.instantiate(SimpleInstantiationStrategy.java:139) ~[spring-beans-6.0.8.jar:6.0.8]
	... 169 common frames omitted

@nidhi-nair
Copy link
Contributor Author

nidhi-nair commented Jun 5, 2023

Looks like this is because we're using a yaml parser to figure out ff4j flags. I can convert this to use an XML parser, it's been a while since the related PR on the ff4j repo has seen any activity. Do you folks think we should go ahead with conversion to XML? @sharat87 @mohanarpit

@sharat87
Copy link
Member

sharat87 commented Jun 6, 2023

@nidhi-nair, thanks. I like this. Irrespective of the priority of this dependency upgrade, and irrespective of when FF4J team decide to release support for the upgrade, I'd vote for moving to XML. As much as I prefer YAML syntax to XML, I have to admit that XML parsers are a lot more rock-solid and battle tested, than anything else. Can we make this permanent? Then perhaps we can just exclude the SnakeYAML dependency completely?

@nidhi-nair
Copy link
Contributor Author

/build-deploy-preview skip-tests=true

@github-actions
Copy link

github-actions bot commented Jun 7, 2023

Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/5196098338.
Workflow: On demand build Docker image and deploy preview.
skip-tests: true.
env: ``.
PR: 23572.
recreate: .

@github-actions
Copy link

github-actions bot commented Jun 7, 2023

Deploy-Preview-URL: https://ce-23572.dp.appsmith.com

@nidhi-nair nidhi-nair requested a review from sharat87 June 8, 2023 02:27
@nidhi-nair
Copy link
Contributor Author

/ok-to-test

@github-actions
Copy link

github-actions bot commented Jun 8, 2023

Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/5206725181.
Workflow: Appsmith External Integration Test Workflow.
Commit: ``.
PR: 23572.
Perf tests will be available at https://app.appsmith.com/app/performance-infra-dashboard/pr-details-638dd7cd2913ba43778b915e?pr=23572&runId=5206725181_1

@github-actions
Copy link

github-actions bot commented Jun 8, 2023

Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/5206725181.
Commit: ``.
The following are new failures, please fix them before merging the PR:

  1. cypress/e2e/Regression/ClientSide/OneClickBinding/TableWidget/mongoDB_spec.ts

To know the list of identified flaky tests - Refer here

@github-actions
Copy link

github-actions bot commented Jun 8, 2023

Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/5206725181.
Commit: ``.
All cypress tests have passed 🎉

@nidhi-nair nidhi-nair merged commit 6e7c293 into release Jun 8, 2023
9 of 10 checks passed
@nidhi-nair nidhi-nair deleted the fix/snake-yaml-upgrade branch June 8, 2023 07:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backend This marks the issue or pull request to reference server code skip-changelog Adding this label to a PR prevents it from being listed in the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants