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: Add log when info.json
is unavailable at specified path
#33293
base: release
Are you sure you want to change the base?
Conversation
WalkthroughWalkthroughThis update enhances error reporting in the Changes
Recent Review DetailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
} | ||
} catch (IOException e) { | ||
// Ignore the exception and return "UNKNOWN" as the version | ||
log.debug("Error reading version from info.json {}", e.getMessage()); | ||
log.error("Error reading version from info.json {}", e.getMessage(), e); |
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.
- I'd recommend not adding
, e
to this. It'll print the whole stack trace and I don't think that'll give you more information than whate.getMessage()
would. - I'd also recommend removing the
if
check, instead of adding anelse
. If the file isn't there, let it throw an error and we're catchingIOException
anyway. In fact, I'd rather the backend not start at all ifinfo.json
is missing, but that makes local dev less convenient.
By the way, the entrypoint.sh
already has a hard-check to fail if the info.json
file is missing.
tr '\n' ' ' < /opt/appsmith/info.json |
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.
@sharat87 if we already have the check in place for info.json then do you think there is a scenario where version does not get populated. I'm asking this because in license check call in EE we refer to the same variable https://github.com/appsmithorg/appsmith-ee/blob/5aa31d0e6162c55a13a79072f33cfd4df24cdc61/app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/BaseLicenseAPIManagerImpl.java#L29. For some reason we have seen that version is communicated as UNKNOWN
which is a default value.
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.
I'd recommend not adding , e to this. It'll print the whole stack trace and I don't think that'll give you more information than what e.getMessage() would.
@sharat87 currently this only prints the info.json path nothing around what failed.
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.
Perhaps the license check is happening before the JSON file is read? Is there a possibility of a race condition here? If we can make the dependency explicit, then that question will go away too.
Other than that, I don't see why this file should be missing or invalid.
Failed server tests
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
No description provided.