-
Notifications
You must be signed in to change notification settings - Fork 52
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
Config changes #47
base: master
Are you sure you want to change the base?
Config changes #47
Conversation
@@ -1,4 +1,4 @@ | |||
### NetBackup API Code Samples for PowerShell | |||
#### NetBackup API Code Samples for PowerShell |
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.
Why reduce the header level? The first header should be level 1, not 3 or 4.
query_params = { | ||
# "page[limit]": 100, #This changes the default page size to 100 | ||
# "filter": "jobType eq 'RESTORE'" #This adds a filter to only show RESTORE Jobs |
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.
Are these parameters valid for this API? If so, then I think there should be a comment explaining that the programmer can uncomment these lines to use them. If they're not valid, then just delete them instead of leaving them commented out.
$json = '{ | ||
"data": { | ||
"type": "trustedMasterServer", | ||
"attributes": { | ||
"trustedMasterServerName": "'+$TrustedMasterServerName+'", |
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 think this and the similar code at 174 should do like is done in Delete-NB-delete-trust.ps1, which creates a hash table and then converts it to JSON with ConvertTo-Json
. It avoids the awkwardness of terminating the string literal midway through, inserting a variable (which hasn't been escaped for use as a JSON string), and then resuming the string.
# Force TLS v1.2 | ||
try { | ||
if ([Net.ServicePointManager]::SecurityProtocol -notcontains 'Tls12') { | ||
[Net.ServicePointManager]::SecurityProtocol += [Net.SecurityProtocolType]::Tls12 |
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.
The +=
notation is apparently only available in Powershell 5. Other examples in this repo instead combine flags with -bor
.
Write-Host "Trust between masters deleted successfully.`n" | ||
Write-Host $response_delete | ||
|
||
$response_delete = (ConvertFrom-Json -InputObject $response_delete) |
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 the result from ConvertFrom-Json
supposed to be used afterward?
Please end text files with a newline.
echo $response_update | ||
Write-Host $response_update |
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.
Don't these both do the same thing?
import json | ||
import texttable as tt |
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.
Neither of these modules is used here.
# "page[limit]": 100, #This changes the default page size to 100 | ||
# "filter": "jobType eq 'RESTORE'" #This adds a filter to only show RESTORE Jobs |
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.
Again, the purpose of the commented-out parameters is unclear. Either remove or explain, please.
Added few APIs in config module.