ngsild-tenant header support | Fix for issue #669 & #664

rohit-vrrr commented 1 year ago

Proposed changes

Describe the big picture of your changes here to communicate to the maintainers why we should accept this pull request. If it fixes a bug or resolves a feature request, be sure to link to that issue.

Types of changes

What types of changes does your code introduce to the project? Put an x in the boxes that apply

[x] Bugfix (non-breaking change which fixes an issue)
[x] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
[ ] Maintenance (update of libraries or other dependences)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

[x] I have read the CONTRIBUTING doc
[x] I have signed the CLA
[x] I have added tests that prove my fix is effective or that my feature works
[x] I have run all the existing tests locally (not just those related to my feature) and there are no errors
[x] After the last push to the PR branch, I have run the lint script locally and there are no changes to the code base
[x] I have updated the RELEASE NOTES
[x] I have added necessary documentation (if appropriate)
[x] Any dependent changes have been merged and published in downstream modules

Further comments

If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...

"Martel Open Source Software Individual Contributor License Agreement"

"Contributing to QuantumLeap"

"QuantumLeap Release Notes"

github-actions[bot] commented 1 year ago

CLA Assistant Lite bot All contributors have signed the CLA ✍️

chicco785 commented 1 year ago

@rohit-vrrr rather than adding a new field in the database, given that the ngsi-ld tenant maps to fiware_service concept i would simply reuse da parameter and use an "or" to populate it based on what is passed in the header

rohit-vrrr commented 1 year ago

@chicco785 Thank you for the suggestion, that would solve a part of the issue. But what if we are passing both the headers 'fiware_service' as well as 'ngsild-tenant' with both unique values. Then how can we know which to consider? So to handle that, I have set 'LD' config in 'src/server/init.py' which determines if we are implementing NGSI or NGSI-LD.

chicco785 commented 1 year ago

@chicco785 Thank you for the suggestion, that would solve a part of the issue. But what if we are passing both the headers 'fiware_service' as well as 'ngsild-tenant' with both unique values. Then how can we know which to consider? So to handle that, I have set 'LD' config in 'src/server/init.py' which determines if we are implementing NGSI or NGSI-LD.

it should not be possible to have both... it's either an ngsild notification or a fiware_service one. I would say that if you have both, ngsild wins over the other

rohit-vrrr commented 1 year ago

I have read the CLA Document and I hereby sign the CLA

chicco785 commented 1 year ago

sweet, so test are working again:

run lint.py.sh to lint the code
add a simple test to be sure your new feature works as planned (for example you can make a copy of this test, and replace the notify_header method with a new one called notify_ld_header or something like that)

c0c0n3 commented 1 year ago

Hi @rohit-vrrr and thanks sooo much for your contrib. Please give us a shout when you think we can review this PR

rohit-vrrr commented 1 year ago

Hi @c0c0n3 ! Yes, this PR is good to go for review. I had actually requested review from @chicco785 , but I didn't get back any response.

c0c0n3 commented 1 year ago

Hi @rohit-vrrr :-)

It looks like this PR has stalled? We'd like to merge your feature quite soon b/c it could help quite a few of our users---e.g. see #719. But there's still a couple of small things left to do

implement the refactoring suggested above
rebase on master and push again

We understand if you're too busy to work on this, no problem. In that case though, can I wrap up this feature myself? You'll obviously get credit for the whole feature in the release notes which will also have a link to this PR. Do I have your permission?

Thanks alot!!

rohit-vrrr commented 1 year ago

Hi @c0c0n3

Sorry for the late response. I am currently occupied with other projects and for me to setup python environment might take some time. So, yeah! you have my permission. 😄 You can go ahead and wrap up this feature.

Thank you!

orchestracities / ngsi-timeseries-api