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

glossary alignment #40

Merged

Conversation

monamok
Copy link
Collaborator

@monamok monamok commented Jun 6, 2023

What type of PR is this?

  • fix

What this PR does / why we need it:

The parameters naming alignment with Glossary

Which issue(s) this PR fixes:

Fixes (#41)

Special notes for reviewers:

  • The parameters names were indicated in Glossary in order to be unified in the whole CAMARA project. This PR is to change msisdn to phoneNumber.
  • Also the example of phoneNumber was change to make it more clear that country code must be included.
  • Some typo fixes and minor changes were included

Changelog input

Changelogs will be included once #28 in accepted and merged

Additional documentation

bigludo7
bigludo7 previously approved these changes Jun 13, 2023
Copy link
Collaborator

@bigludo7 bigludo7 left a comment

Choose a reason for hiding this comment

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

Works for me.
We will introduce the UTC precision later?

@monamok
Copy link
Collaborator Author

monamok commented Jun 13, 2023

We will introduce the UTC precision later?

We can include it in the PR we have to open for #42

@bigludo7 bigludo7 self-requested a review June 13, 2023 15:18
Copy link
Collaborator

@bigludo7 bigludo7 left a comment

Choose a reason for hiding this comment

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

Added UTC precision + error code 404 to accomodate both issue open.

@monamok monamok requested a review from bigludo7 June 14, 2023 12:59
@monamok
Copy link
Collaborator Author

monamok commented Jun 14, 2023

Added UTC precision

@bigludo7 are we going to wait for the commonalities issue about this to get resolved or are we going to merge this PR and then align it with the decision afterwards?

@bigludo7
Copy link
Collaborator

@monamok I suggest to wait tomorrow Commonalities call.
If the team there pick an option during the call we can accommodate our api accordingly.
If no decision, we remove the UTC precision from our API and we merge without it.

Probably good to inform @DT-DawidWroblewski. I should not be able to connect tomorrow to the Commonalities call so hope someone can trigger the discussion (Dawid, you or @gregory1g )
Work for you?

@monamok
Copy link
Collaborator Author

monamok commented Jun 14, 2023

@bigludo7 ok, we can wait to commonalities meeting.
Should I include what we talked in #42 about removing Client Credentials and leave only 3-legged in this PR too? Or do you prefer to open a new PR for that?

@monamok
Copy link
Collaborator Author

monamok commented Jun 14, 2023

@bigludo7 I removed UTC for the moment so if you approve and @DT-DawidWroblewski agrees too, we can merge and close this PR tomorrow during the bi-weekly meeting.
For the other two issues I will open a new PR afterwards.

bigludo7
bigludo7 previously approved these changes Jun 14, 2023
Copy link
Collaborator

@bigludo7 bigludo7 left a comment

Choose a reason for hiding this comment

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

OK work for me as well.
We can create another PR for UTC after.
Let's go for this

@monamok
Copy link
Collaborator Author

monamok commented Jun 14, 2023

@bigludo7 thank you.

Copy link
Collaborator

@DT-DawidWroblewski DT-DawidWroblewski left a comment

Choose a reason for hiding this comment

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

all good

@DT-DawidWroblewski DT-DawidWroblewski merged commit d0e6d32 into camaraproject:main Jun 15, 2023
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.

3 participants