-
-
Notifications
You must be signed in to change notification settings - Fork 19
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
feat(ios): Add iOS organizations to Certificate picker #1244
base: master
Are you sure you want to change the base?
Conversation
@@ -480,7 +476,12 @@ | |||
"titanium.project.defaultI18nLanguage": { | |||
"type": "string", | |||
"default": "en", | |||
"description": "%titanium.config.project.defaultI18nLanguage%" | |||
"description": "Default language to use for i18n autocomplete." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason we've switched this to not use the localization id?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know, I didn't change that - you did on March 1st 2019 -
[Ewan Harris]5 years ago (March 1st, 2019 5:38 PM)
docs(configuration): add script to generate config settings table, link to it from readme
Closes [#49]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the current code we have what was removed, it should be "%titanium.config.project.defaultI18nLanguage%"
as that's what it was changed to in this PR.
Maybe the branch isn't up to date with this repo so doesn't include that initial change?
"titanium.ios.organizations": { | ||
"type": "string", | ||
"default": "", | ||
"description": "Input JSON object with your organization's names and IDs. " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add this into package.nls.json
and then reference using that ID so that it's setup for localization in future..I think titanium.config.ios.organizations
should be suitable.
Alongside that could we also document what the expected string should look like?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
`[
{
"id": "TZF9D6738F",//organisation ID from certificate/apple developer/etc
"name": "1st Organisation" //name to show, can be anything
},
{
"id": "73QRFU8MDC",
"name": "Second one"
},
]`
Is this okay?
To type change - I tried it with various types, but it throwed errors, so I stayed with string
I also added entry to package.nls.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we used "type": "object'
then we can get a table like below in the settings UI that would accept a key/value data, the certificate ID could be the key and the value could be the custom name. My concern with JSON in a string is that editing isn't really simple and we would have to handle data validation and error reporting in the case of bad input.
The table is returned as an object so would change how we access but that shouldn't be an issue.
{
"TZF9D6738F": "1st Organisation"
"73QRFU8MDC": "Second one"
}
"description": "Default language to use for i18n autocomplete." | ||
}, | ||
"titanium.ios.organizations": { | ||
"type": "string", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to use object
instead of string
here to allow folks to do a key/value as configuration? That might simplify how we look up the organization also
"titanium.ios.organizations": {
"certId1": "Acme"
}
const configs = vscode.workspace.getConfiguration('titanium.ios'); | ||
const organizations = JSON.parse(configs.organizations); | ||
ExtensionContainer.environment.iOSCertificates(certificateType).forEach(cert => { | ||
let organization = cert.name.split('(')[1]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I unfortunately don't have any iOS certs to test, but I'm curious if the value we're extracting from the brackets is available in any of the data ioslib sets, I think the type we use in here is partially implemented to just what we use.
If you run ti info -t ios -o json
you can check to see if it's possible to use it and then update the type to include it
feat(all): added entry to package.nls.json
When you need to pick up right certificate to build iOS app, you see just name. With newly option you can specify via JSON object which certificate id belongs to which organization. It's very usefull if you are developer for various clients...