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

Rename options #4467

Closed
29 of 30 tasks
milanholemans opened this issue Feb 4, 2023 · 26 comments
Closed
29 of 30 tasks

Rename options #4467

milanholemans opened this issue Feb 4, 2023 · 26 comments

Comments

@milanholemans
Copy link
Contributor

milanholemans commented Feb 4, 2023

This issue is created to keep track of options we should update/rename for the next major v7 release.

aad:

  • aad user set
    • Rename objectId to id
    • Rename userPrincipalName to userName

pa:

  • pa app export
    • Rename id to -n, --name

  • pa app consent set
  • pa app export
  • pa app list
    • Rename environment to environmentName

pp:

  • pa aibuildermodel get
  • pa aibuildermodel list
  • pa aibuildermodel remove
  • pp card clone
  • pp card get
  • pp card list
  • pp card remove
  • pp chatbot get
  • pp chatbot list
  • pp chatbot remove
  • pp dataverse table get
  • pp dataverse table list
  • pp dataverse table remove
  • pp dataverse table row list
  • pp dataverse table row remove
  • pp solution get
  • pp solution list
  • pp solution publish
  • pp solution remove
  • pp solution publisher get
  • pp solution publisher add
  • pp solution publisher list
  • pp solution publisher remove
    • Rename environment to environmentName

spo:

  • spo listitem attachment list
    • Rename itemId to listItemId
  • spo user get
    • Rename loginName to userName

IMPORTANT

Let's recheck the script samples and update them with the new options.

@milanholemans milanholemans added this to the v7 milestone Feb 4, 2023
@martinlingstuyl
Copy link
Contributor

Hi @milanholemans, userName is something different from userDisplayName, as we always use it to mean the UPN of a user.

I think in this case we might add a userName option and keep the userDisplayName as well. (Eg an optionSet)

@milanholemans
Copy link
Contributor Author

Ur right, seems like I was a bit too fast here.

@milanholemans milanholemans removed this from the v7 milestone Feb 5, 2023
@milanholemans milanholemans closed this as not planned Won't fix, can't repro, duplicate, stale Feb 5, 2023
@milanholemans milanholemans reopened this Feb 11, 2023
@milanholemans milanholemans added this to the v7 milestone Feb 11, 2023
@milanholemans
Copy link
Contributor Author

Revived this issue because I found some other options that are not consistent/don't match our naming convention.

@martinlingstuyl
Copy link
Contributor

I think renaming userName to name makes it less clear though, @milanholemans. userName is a thing in itself (upn). While name can be a displayName.

I'd say: rename userPrincipalName to userName.

I'd personally keep objectId as-is as well. That's a thing as well, which we use for apps as well. But as we're using userId elsewhere, I can understand it would be smart to rename it to id here. Not sure though...

Other opinions on this?

@milanholemans
Copy link
Contributor Author

milanholemans commented Feb 12, 2023

This is one of the few places where we use objectId instead of userId or id. According to me, objectId is not much clearer than just id.

@martinlingstuyl
Copy link
Contributor

Well, it's how it is called in Azure AD. But I agree for a user it might be clearer to use id, as we're oftentimes referring to it as userId.

@Jwaegebaert
Copy link
Contributor

I would prefer id in this scenario. We are talking about an userId here so id would suffice as it reflects on the command.

@waldekmastykarz
Copy link
Member

Does as user in AAD have another ID than the objectId (which would be the same as the user ID)?

@milanholemans
Copy link
Contributor Author

Does as user in AAD have another ID than the objectId (which would be the same as the user ID)?

According to me, AAD user ID is the same as objectId. Just a different name that is only used in AAD on the user profile.

@waldekmastykarz
Copy link
Member

If they're the same, I suggest we use id

@martinlingstuyl
Copy link
Contributor

Opening this up as we are releasing v7 after the summer holidays. Warmly recommended for your contribution. 🎉
Target the v7 branch when raising a PR.

@milanholemans
Copy link
Contributor Author

@pnp/cli-for-microsoft-365-maintainers seems like we have 26 flow & pa commands that use environment as an option. 16 other commands are using environmentName.
I think we should align them a bit more and use the same option name. Which one should we keep?

@martinlingstuyl
Copy link
Contributor

Hi @milanholemans, I think I'd favor environmentName.

@milanholemans
Copy link
Contributor Author

Hi @milanholemans, I think I'd favor environmentName.

Me too, looking at PS commands, it seems like they are using environmentName as well.

@appieschot
Copy link
Member

Me too, looking at PS commands, it seems like they are using environmentName as well.

I don't have a clear preference as long as it is consistent. The pac cli uses environment :
https://learn.microsoft.com/en-us/power-platform/developer/cli/reference/pipeline#pac-pipeline-list / https://learn.microsoft.com/en-us/power-platform/developer/cli/reference/admin#pac-admin-backup

@martinlingstuyl
Copy link
Contributor

The pac cli uses environment

It does not seem very consistent, as it does use solutionName...

I think environmentName is clearer, as it's immediately clear what value is expected.

@appieschot
Copy link
Member

Guess it's semantics, for envs they allow providing the url so it wouldn't make sense there.

@Jwaegebaert
Copy link
Contributor

Jwaegebaert commented Jul 5, 2023

Good catch @milanholemans, let's streamline this. I'm also fan of using environmentName instead

@milanholemans
Copy link
Contributor Author

All right I'll add the commands that are using environment to this issue.

@Saurabh7019
Copy link
Contributor

This issue seems like an excellent opportunity to become familiar with a lot of commands and script samples. Can I work on it?

@milanholemans
Copy link
Contributor Author

Sure @Saurabh7019! Thanks!

@milanholemans
Copy link
Contributor Author

Hi @Saurabh7019 FYI, I added another name change regarding pa app export.

@Jwaegebaert
Copy link
Contributor

I just came across spo listitem attachment list that uses --itemId <itemId> for the list item id. I would suggest changing this to --listItemId <listItemId>

@milanholemans
Copy link
Contributor Author

@Jwaegebaert sure makes sense, please add it to the list 😄

@Saurabh7019
Copy link
Contributor

Hi @Saurabh7019 FYI, I added another name change regarding pa app export.

Hi Milan, I wanted to clarify that changing the parameter name from "--id" to "--name" without making changes to the code could create confusion for users. After reviewing the code, I noticed that it actually requires the app GUID in order to export the app, rather than the app name.

@milanholemans
Copy link
Contributor Author

Hi Milan, I wanted to clarify that changing the parameter name from "--id" to "--name" without making changes to the code could create confusion for users. After reviewing the code, I noticed that it actually requires the app GUID in order to export the app, rather than the app name.

Hi @Saurabh7019 you are right. However all other Power App commands are using name to specify the ID of the app. Looking at the Power Apps PowerShell module, they are also using name to specify the ID.

I internally started a discussion to change all name options to id because we're always expecting an ID. I will try to reopen the discussion because in my opinion we should use id instead of name everywhere.
For now you can ignore this command.

Saurabh7019 added a commit to Saurabh7019/cli-microsoft365 that referenced this issue Jul 15, 2023
Saurabh7019 added a commit to Saurabh7019/cli-microsoft365 that referenced this issue Jul 15, 2023
Saurabh7019 added a commit to Saurabh7019/cli-microsoft365 that referenced this issue Jul 16, 2023
@milanholemans milanholemans linked a pull request Jul 18, 2023 that will close this issue
Saurabh7019 added a commit to Saurabh7019/cli-microsoft365 that referenced this issue Jul 24, 2023
milanholemans pushed a commit that referenced this issue Jul 24, 2023
milanholemans pushed a commit that referenced this issue Aug 6, 2023
milanholemans pushed a commit that referenced this issue Aug 12, 2023
Adam-it pushed a commit that referenced this issue Aug 16, 2023
Jwaegebaert pushed a commit that referenced this issue Aug 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants