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

Feature/not fail on receiving with unknown ocpp properties #1460

Conversation

dakai-wei-of-shizen
Copy link
Contributor

@dakai-wei-of-shizen dakai-wei-of-shizen commented May 24, 2024

we are trying to use SteVe to provide customer the OCPP Server, facing these problems and solving it.

  • some CP added custom ocpp fields(properties) in their request from CP or response, mostly for debug or exception handling, making the steve ocpp server not working as expected.
    =>So I made it not to fail on receiving with unknown ocpp properties
  • (implemented BUT REVERSED)some of our customers' CP can only set IP and Port in their CP, making the websocket endpoint not accessible .
    =>So I added a reverse proxy to handle this locally first.
    =>Not related to the title of the Pull Request so it is REVERSED.

@goekay
Copy link
Member

goekay commented May 26, 2024

thanks for the contribution. can you pls write a description and motivation for the changes? why are you adding nginx? this PR is mixing two concerns.

@dakai-wei-of-shizen dakai-wei-of-shizen changed the title Feature/not fail on unknown properties Feature/not fail on receiving with unknown ocpp properties May 27, 2024
@dakai-wei-of-shizen
Copy link
Contributor Author

dakai-wei-of-shizen commented May 27, 2024

thanks for the contribution. can you pls write a description and motivation for the changes? why are you adding nginx? this PR is mixing two concerns.

Thanks for your remind!
I updated the brief of this PR
I didn't intend to open a PR in this origin repo, the diff is only for dev local env, I can close it. Sorry for the disturbing.

Do you think it is meaningful for others? I can update related README adding a line of explanation or the nginx URL if the comunity need it.

@dakai-wei-of-shizen dakai-wei-of-shizen marked this pull request as draft May 27, 2024 01:20
@goekay
Copy link
Member

goekay commented Jun 2, 2024

hey there, thanks for the updates and motivation.

i find both of these optional, but i like one better than the other.

  • nginx: this is too close to infra decisions. as you can see, the codebase of steve does not tell/dictate anything how your infra should look like and how you should install steve. we leave infra-relation decisions to the user, because there are many variants. makes sense?
  • custom msg fields: if they add custom fields to messages, this would mean that they do NOT create ocpp-compliant messages anymore. this is bad for the standard. "strict me" would say no, but i can understand where it is coming from. since this is not a destructional change, i am okay with having this.

@dakai-wei-of-shizen
Copy link
Contributor Author

@goekay

nginx:

Got it.

custom msg fields:

Understood. I will make it configureable in the main.properties and recreate the PR.

Closing this PR.

@goekay
Copy link
Member

goekay commented Jun 3, 2024

i am actually fine with the current state of custom msg fields. no need for additional configuration.

i just would like to omit the nginx changes.

@dakai-wei-of-shizen dakai-wei-of-shizen marked this pull request as ready for review June 3, 2024 12:38
@dakai-wei-of-shizen
Copy link
Contributor Author

@goekay
As you wish!
Made it ready for review.

@dakai-wei-of-shizen dakai-wei-of-shizen deleted the feature/not-fail-on-unknown-properties branch June 3, 2024 12:48
@dakai-wei-of-shizen dakai-wei-of-shizen restored the feature/not-fail-on-unknown-properties branch June 4, 2024 00:31
@goekay goekay merged commit 8605b61 into steve-community:master Jun 19, 2024
23 checks passed
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 this pull request may close these issues.

None yet

2 participants