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

fix: Server does not start via CLI when auth option is set #8669

Merged
merged 3 commits into from
Jun 29, 2023

Conversation

dblythy
Copy link
Member

@dblythy dblythy commented Jun 27, 2023

Pull Request

Issue

Closes: #8665

Approach

See Related: #8666.

@parse-github-assistant
Copy link

parse-github-assistant bot commented Jun 27, 2023

Thanks for opening this pull request!

@dblythy dblythy changed the base branch from alpha to release-5.x.x June 27, 2023 04:37
@dblythy
Copy link
Member Author

dblythy commented Jun 27, 2023

@FransGH please try this one out:

npm install dblythy/parse-server#fix-auth-parser-lts

@dblythy dblythy changed the title Fix auth parser lts fix: (LTS) Auth does not load when starting Parse Server via CLI Jun 27, 2023
@dblythy dblythy closed this Jun 27, 2023
@dblythy dblythy reopened this Jun 27, 2023
@codecov
Copy link

codecov bot commented Jun 27, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (a036071) 94.12% compared to head (d30d814) 94.12%.

❗ Current head d30d814 differs from pull request most recent head 78417ea. Consider uploading reports for the commit 78417ea to get more accurate results

Additional details and impacted files
@@              Coverage Diff               @@
##           release-5.x.x    #8669   +/-   ##
==============================================
  Coverage          94.12%   94.12%           
==============================================
  Files                183      183           
  Lines              13798    13803    +5     
==============================================
+ Hits               12987    12992    +5     
  Misses               811      811           
Impacted Files Coverage Δ
src/Options/Definitions.js 100.00% <ø> (ø)
src/Options/index.js 100.00% <ø> (ø)
src/Controllers/DatabaseController.js 93.96% <100.00%> (+0.05%) ⬆️
src/RestWrite.js 94.61% <100.00%> (-0.04%) ⬇️
src/Routers/FilesRouter.js 93.25% <100.00%> (-0.05%) ⬇️
src/Utils.js 98.13% <100.00%> (+0.09%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@FransGH
Copy link
Contributor

FransGH commented Jun 27, 2023

Just deployed dblythy/parse-server#fix-auth-parser-lts to staging and all seems to work fine!

Thanks Daniel for addressing this so quickly :)

@dblythy
Copy link
Member Author

dblythy commented Jun 27, 2023

Thanks for reviewing @FransGH!

@dblythy dblythy requested a review from a team June 27, 2023 12:16
@mtrezza
Copy link
Member

mtrezza commented Jun 27, 2023

@dblythy could you please add the fix to the PR description, and also a link to the alpha branch PR (if there is one)? Thanks!

@dblythy
Copy link
Member Author

dblythy commented Jun 27, 2023

Sorry, the alpha PR is #8666

@mtrezza mtrezza changed the title fix: (LTS) Auth does not load when starting Parse Server via CLI fix: Auth does not load when starting Parse Server via CLI Jun 27, 2023
@FransGH
Copy link
Contributor

FransGH commented Jun 27, 2023

@mtrezza not the most important but the problem is that the server fails to start via CLI when auth is configured (as opposed to just auth-adapters not loading)

@mtrezza
Copy link
Member

mtrezza commented Jun 27, 2023

Thanks for pointing this out, the PR title will be the changelog entry, so it's important we get this right. The server fails to start via CLI when auth is configured as a CLI parameter? If so, how about this title:

fix: Server does not start via CLI with --auth parameter set

@FransGH
Copy link
Contributor

FransGH commented Jun 27, 2023

I didn't try via --auth, only via a config.json. Assume however that it's independent of where the CLI gets the config from. Config file, cmd parameters and env-variables should have the same checks.

@mtrezza
Copy link
Member

mtrezza commented Jun 27, 2023

So you were using a config file and were passing that via the CLI?

@FransGH
Copy link
Contributor

FransGH commented Jun 27, 2023

Yes, but only on staging and production. During dev a server instance is started via API where apparently the config is not so strictly checked.

@mtrezza
Copy link
Member

mtrezza commented Jun 27, 2023

Got it, different checks for CLI vs. API doesn't sound good. Do you want to open an issue to centralize the checks for config options regardless of how they are set? Sounds like a general issue.

How about this:

fix: Server does not start via CLI when passing a config file with auth parameter set

@FransGH
Copy link
Contributor

FransGH commented Jun 27, 2023

Thought about opening an issue for that but don't have time at the moment to contribute in fixing it. So not sure if it makes sense creating one?

@FransGH
Copy link
Contributor

FransGH commented Jun 27, 2023

fix: Server does not start via CLI when passing a config file with auth parameter set

"Fix: Server does not start via CLI when the auth option is set."

@mtrezza
Copy link
Member

mtrezza commented Jun 28, 2023

Thought about opening an issue for that but don't have time at the moment to contribute in fixing it. So not sure if it makes sense creating one?

Sure, you don't need to contribute in fixing an issue - if you only want to report it that also helps.

@mtrezza mtrezza changed the title fix: Auth does not load when starting Parse Server via CLI fix: Server does not start via CLI when the auth option is set Jun 28, 2023
@mtrezza mtrezza changed the title fix: Server does not start via CLI when the auth option is set fix: Server does not start via CLI when auth option is set Jun 28, 2023
@FransGH
Copy link
Contributor

FransGH commented Jun 28, 2023

@mtrezza just for my understanding, what would be the approximate timeline for seeing this in a 5.x release update?
(I can work around this but would rather upgrade with an official release)

spec/CLI.spec.js Outdated Show resolved Hide resolved
@dblythy dblythy requested a review from a team June 29, 2023 04:57
@mtrezza mtrezza merged commit 601da1e into parse-community:release-5.x.x Jun 29, 2023
Copy link
Member

@mtrezza mtrezza left a comment

Choose a reason for hiding this comment

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

Looks good!

parseplatformorg pushed a commit that referenced this pull request Jun 29, 2023
## [5.5.3](5.5.2...5.5.3) (2023-06-29)

### Bug Fixes

* Server does not start via CLI when `auth` option is set ([#8669](#8669)) ([601da1e](601da1e))
@parseplatformorg
Copy link
Contributor

🎉 This change has been released in version 5.5.3

@parseplatformorg parseplatformorg added the state:released-5.x.x Released as LTS version label Jun 29, 2023
@dblythy dblythy deleted the fix-auth-parser-lts branch June 30, 2023 03:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state:released-5.x.x Released as LTS version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants