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

Change data value encoding to Base64 URL-safe #1326

Conversation

adamsero
Copy link
Contributor

No description provided.

@adamsero
Copy link
Contributor Author

PR for #1325

@sbernard31
Copy link
Contributor

  1. some concern about decoding, we started to discuss about it at : Data Value in SenML format is not encoded in URL-safe way #1325 (comment)

  2. I think we should add tests about this (integration or unit test)

  3. About issue reference in commit message, until recently I was using #1325 but it is sometime painful because if you start the commit header with # sometime it can be consider as a commented line (see https://stackoverflow.com/questions/2788092/start-a-git-commit-message-with-a-hashmark#2788642) So recently I decide to use GH-1325 instead of #1325 in commit message. (see https://docs.github.com/en/get-started/writing-on-github/working-with-advanced-formatting/autolinked-references-and-urls#issues-and-pull-requests).
    If you think this is a good idea, please replace eclipse#1325 by GH-1325.

@adamsero adamsero force-pushed the opl/url_safe_data_value_encoding branch from 8781948 to b35304b Compare November 23, 2022 11:03
Copy link
Contributor

@sbernard31 sbernard31 left a comment

Choose a reason for hiding this comment

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

I think we should add some test about base64 decoder /encoder.

Maybe with case raised at : #676 (comment)

@sbernard31
Copy link
Contributor

You didn't answer to :

I think we should add some test about base64 decoder /encoder.
Maybe with case raised at : #676 (comment)

So I don't know if you plan to do it or not ? 🙂

@adamsero
Copy link
Contributor Author

adamsero commented Dec 1, 2022

@sbernard31 Just want to let you know that @Warmek will finish this pull request as well as any future tasks as I'm leaving Orange and the project. Thank you for your time spent on helping me learn the code and the project overall 🙂.

@sbernard31
Copy link
Contributor

sbernard31 commented Dec 1, 2022

@adamsero, never a good news to see contributor go. I hope this is mainly a desired choice from you.
Thanks for all your contributions and good luck for the future. 🙏

If you have any feedback about how we worked together on the project (good or bad things) please do not hesitate to share. This could help to improve. (you can either use github or send me a private mail as you prefer 🙂)

@sbernard31
Copy link
Contributor

@Warmek do no hesitate to ask if you face any issue / difficulty on this 😉

@sbernard31
Copy link
Contributor

sbernard31 commented Dec 6, 2022

@Warmek let me know when this is in a reviewable state :)

@Warmek
Copy link
Contributor

Warmek commented Dec 6, 2022

@sbernard31 I think i've done it and it's ready for your review

@sbernard31
Copy link
Contributor

@Warmek, I will take some time to get deeper about the charset encoding question. (just to be sure I well understand the problem)

Then I plan to work a bit more on this :

  • removing org.eclipse.leshan.core.util.Base64 class
  • maybe add more test and validation in base64 codec.
  • maybe add a decoder which is able to support all format for LWM2M old-json Content Format.

I let you know when I have code to review.

Copy link
Contributor

@Warmek Warmek left a comment

Choose a reason for hiding this comment

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

👍

@Warmek
Copy link
Contributor

Warmek commented Dec 8, 2022

@sbernard31 Do you think it will be possibile to share leshan M10 in first half of december?

@sbernard31 sbernard31 mentioned this pull request Dec 8, 2022
3 tasks
@sbernard31
Copy link
Contributor

Do you think it will be possibile to share leshan M10 in first half of december?

I'm not sure what means "share" ?
If you talk about a release, we can try but this means we have just 1 week more left.

And after my proposition at : #1319 (comment)
You (at Orange) asked in an email for more time to test before the release :

We as Orange would prefer to know a week ahead before a milestone is released so we can test all the features for possible bugs.

You (Orange) also proposes :

Maybe we could create a separate issue for that purpose or organize it differently if you'd like.

So I created an issue talk about 2.0.0-M10 release (as I did for 2.0.0-M9) : #1370

Copy link
Contributor

@Warmek Warmek left a comment

Choose a reason for hiding this comment

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

👍

@Warmek
Copy link
Contributor

Warmek commented Dec 12, 2022

@sbernard31 I reviewed your changes and i wait for merge

@sbernard31
Copy link
Contributor

This is now integrated in #1371, so I close this PR.

Thx @adamsero and @Warmek for your contributions 🙏 !

@sbernard31 sbernard31 closed this Dec 12, 2022
@JaroslawLegierski
Copy link
Contributor

You (at Orange) asked in an email for more time to test before the release :

We as Orange would prefer to know a week ahead before a milestone is released so we can test all the features for possible bugs.

Yes our colleagues are ready to test all new features - as soon as they are available in the master branch.

Thx @adamsero and @Warmek for your contributions 🙏 !

First of all, we thank You for Your help !

@sbernard31
Copy link
Contributor

First of all, we thank You for Your help !

Glad to help 🙂

@sbernard31
Copy link
Contributor

sbernard31 commented Dec 12, 2022

@Warmek @JaroslawLegierski

I fall on this last week : https://www.eclipse.org/projects/handbook/#resources-commit

The email address of the author must match the email address on the Eclipse Foundation Account, the name "Some Body" must be replaced by the real name of the person.

☝️ this is not checked by any eclipse validation tools but would be better if you change your git configuration to match the eclipse requirement.

Currently @Warmek Author looks like Author: Warmek <b***********@orange.com> (which is not so good)
and @JaroslawLegierski looks like this : Author: JaroslawLegierski <j*********@orange.com> (which is almost OK but maybe better with a space)

I let you know but I think this is clearly not critical.

@JaroslawLegierski
Copy link
Contributor

and @JaroslawLegierski looks like this : Author: JaroslawLegierski <j*********@orange.com> (which is almost OK but maybe better with a space)

I modified the Name field in the Github configuration -> now is in the form FirstName space LastName. I'll have to see what is the effect of these changes.

@Warmek
Copy link
Contributor

Warmek commented Feb 27, 2023

I've changed my user.name via command:
git config --global user.name "Some Body"

Will that help?

@sbernard31
Copy link
Contributor

sbernard31 commented Feb 27, 2023

did you commit your last commit (d0861f8) before or after running this ?
(because in your last commit there is still Author: Warmek <b***********@orange.com>)

@Warmek
Copy link
Contributor

Warmek commented Feb 27, 2023

I've made my last commit before running this command

@sbernard31
Copy link
Contributor

OK when you will push a new commit, I will be able to say you if this is OK now. 🙂

@sbernard31
Copy link
Contributor

I checked your last commit (12c96fc) that sounds good now. Thx 🙏

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.

4 participants