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.

Sign up for GitHub

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

Using cmd/c to avoid PowerShell ISE does not accept stderr #473

Merged
merged 4 commits into from
May 26, 2022
Merged

Using cmd/c to avoid PowerShell ISE does not accept stderr #473

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d6381de
Change use cmd/c for creating new shell to package all stderr -> stdout
khanhntd May 21, 2022
a77b984
Add 2>&1 for otel collector and fix some typo
khanhntd May 21, 2022
a306cfa
Add comments for config-translator
khanhntd May 24, 2022
cc79774
Add comment and fix comment for config translator, config downloader …
khanhntd May 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cmd/config-downloader/downloader.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ func main() {
region = util.DetectRegion(mode, cc.CredentialsMap())

if region == "" && downloadLocation != locationDefault {
fmt.Println("Unable to determine aws-region.")
log.Println("E! Unable to determine aws-region.")
if mode == config.ModeEC2 {
errorMessage = fmt.Sprintf("E! Please check if you can access the metadata service. For example, on linux, run 'wget -q -O - http://169.254.169.254/latest/meta-data/instance-id && echo' ")
} else {
Expand Down
32 changes: 15 additions & 17 deletions packaging/windows/amazon-cloudwatch-agent-ctl.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,10 +113,10 @@ $EC2 = $false
$CIM = $false

Function StartAll() {
Write-Output "****** processing cwagent-otel-collector ******"
Write-Output "****** Processing cwagent-otel-collector ******"
AgentStart -service_name $CWOCServiceName -service_display_name $CWOCServiceDisplayName

Write-Output "`r`n****** processing amazon-cloudwatch-agent ******"
Write-Output "`r`n****** Processing amazon-cloudwatch-agent ******"
AgentStart -service_name $CWAServiceName -service_display_name $CWAServiceDisplayName
}

Expand Down Expand Up @@ -169,10 +169,10 @@ Function AgentStart() {
}

Function StopAll() {
Write-Output "****** processing cwagent-otel-collector ******"
Write-Output "****** Processing cwagent-otel-collector ******"
AgentStop -service_name $CWOCServiceName

Write-Output "`r`n****** processing amazon-cloudwatch-agent ******"
Write-Output "`r`n****** Processing amazon-cloudwatch-agent ******"
AgentStop -service_name $CWAServiceName
}

Expand Down Expand Up @@ -356,27 +356,27 @@ Function CWAConfig() {
}

if ($ConfigLocation -eq $AllConfig -And $multi_config -ne 'remove') {
Write-Output "ignore cwa configuration `"$AllConfig`" as it is only supported by action `"remove-config`""
Write-Output "Ignore amazon-cloudwatch-agent's configuration ${AllConfig} as it is only supported by action `"remove-config`""
return
}

if ($ConfigLocation -eq $AllConfig) {
Remove-Item -Path "${JSON_DIR}\*" -Force -ErrorAction SilentlyContinue
} else {
& $CWAProgramFiles\config-downloader.exe --output-dir "${JSON_DIR}" --download-source "${ConfigLocation}" --mode "${param_mode}" --config "${COMMON_CONIG}" --multi-config "${multi_config}"
& cmd /c "`"$CWAProgramFiles\config-downloader.exe`" --output-dir ${JSON_DIR} --download-source ${ConfigLocation} --mode ${param_mode} --config ${COMMON_CONIG} --multi-config ${multi_config} 2>&1"
CheckCMDResult
}

$jsonDirContent = Get-ChildItem "${JSON_DIR}" | Measure-Object

if ($jsonDirContent.count -eq 0) {
Write-Output "all amazon-cloudwatch-agent configurations have been removed"
Write-Output "All amazon-cloudwatch-agent configurations have been removed"
Remove-Item "${TOML}" -Force -ErrorAction SilentlyContinue
} else {
Write-Output "Start configuration validation..."
& cmd /c "`"$CWAProgramFiles\config-translator.exe`" --input ${JSON} --input-dir ${JSON_DIR} --output ${TOML} --mode ${param_mode} --config ${COMMON_CONIG} --multi-config ${multi_config} 2>&1"
CheckCMDResult
# Let command pass so we can check return code and give user-friendly error-message
Copy link
Contributor

Choose a reason for hiding this comment

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

Why remove this comment? It is explaining what setting the $ErrorActionPreference does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Forgot to update the comment on the config-translator since the existing one would not be enough. However, I have fixed it in the newest commit.

Copy link
Contributor

Choose a reason for hiding this comment

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

There's no reason to touch this. Can you just revert the change for the comment here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its for better information right? Just wank to ask if there anything wrong with the new comments. Customer won't look at this file; therefore, its only for dev.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think if you want to include information, what needs to be explained is why the ErrorActionPreference is set here, and not in the line above it for the config translator. If there isn't a clear explanation for that, I don't see value in trying to add more comments here. I also don't see any reason to link to https://www.tutorialspoint.com/how-to-use-the-erroractionpreference-variable-in-powershell

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make sense. I have updated the newest comment regarding these details. Thanks for reminding me and sorry for commenting late.


$ErrorActionPreference = "Continue"
& cmd /c "`"${CWAProgramFiles}\amazon-cloudwatch-agent.exe`" --schematest --config ${TOML} 2>&1" | Out-File $CVLogFile
if ($LASTEXITCODE -ne 0) {
Expand Down Expand Up @@ -425,11 +425,11 @@ Function CWOCConfig() {
)

if (Test-Path -LiteralPath "${Env:ProgramFiles}\Amazon\AWSOTelCollector\aws-otel-collector-ctl.ps1") {
Write-Output "REMINDER: you are configuring `"cwagent-otel-collector`" instead of `"aws-otel-collector`"."
Write-Output "REMINDER: You are configuring `"cwagent-otel-collector`" instead of `"aws-otel-collector`"."
}

if ($multi_config -eq 'append') {
Write-Output "ignore `"-o`" as cwagent-otel-collector doesn't support append-config"
Write-Output "Ignore `"-o`" as cwagent-otel-collector doesn't support append-config"
return
}

Expand All @@ -439,7 +439,7 @@ Function CWOCConfig() {
}

if ($OtelConfigLocation -eq $AllConfig -And $multi_config -ne 'remove') {
Write-Output "ignore cwoc configuration `"$AllConfig`" as it is only supported by action `"remove-config`""
Write-Output "Ignore cwagent-otel-collector's configuration `"$AllConfig`" as it is only supported by action `"remove-config`""
return
}

Expand All @@ -449,15 +449,13 @@ Function CWOCConfig() {
Copy-Item "${PREDEFINED_CONFIG_DATA}" -Destination "${YAML_DIR}/default.tmp"
Write-Output "Successfully fetched the config and saved in ${YAML_DIR}\default.tmp"
} else {
& $CWAProgramFiles\config-downloader.exe --output-dir "${YAML_DIR}" --download-source "${OtelConfigLocation}" --mode "${param_mode}" --config "${COMMON_CONIG}" --multi-config "${multi_config}"
if ($LASTEXITCODE -ne 0) {
return
}
& cmd /c "`"$CWAProgramFiles\config-downloader.exe`" --output-dir ${YAML_DIR} --download-source ${OtelConfigLocation} --mode ${param_mode} --config ${COMMON_CONIG} --multi-config ${multi_config} 2>&1"
Copy link
Contributor

Choose a reason for hiding this comment

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

In the PR overview, change description: it says "using cmd/ c for config-translator". I think you meant to say "using cmd/ c for config-downloader.exe". Nice work figuring out this change fix the issue, before we merge this change, can you do a quick spike regarding understand, why & $CWAProgramFiles\config-downloader.exe ... is used in the first place, while amazon-cloudwatch-agent and config-translator is invoked by using cmd /c? I don't have the context the different between those two, and I think understand this difference better could help us avoid side effects this change may cause.

Copy link
Contributor Author

@khanhntd khanhntd May 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

In the PR overview, change description: it says "using cmd/ c for config-translator". I think you meant to say "using cmd/ c for config-downloader.exe".

Ack. My bad on writing the wrong artifacts.

Nice work figuring out this change fix the issue, before we merge this change, can you do a quick spike regarding understand, why & $CWAProgramFiles\config-downloader.exe ... is used in the first place, while amazon-cloudwatch-agent and config-translator is invoked by using cmd /c

That's a good question. Besides the advantage/disadvatange between using cmd /c and native shell, I have not know why it was used in the first place.

CheckCMDResult
}

$yamlDirContent = Get-ChildItem "${YAML_DIR}" | Measure-Object
if ($yamlDirContent.count -eq 0) {
Write-Output "all cwagent-otel-collector configurations have been removed"
Write-Output "All cwagent-otel-collector configurations have been removed"
Remove-Item "${YAML}" -Force -ErrorAction SilentlyContinue
} else {
# delete old file which are without .tmp suffix
Expand All @@ -469,7 +467,7 @@ Function CWOCConfig() {
$destination = Join-Path -Path $_.Directory.FullName -ChildPath "${newName}"
Move-Item $_.FullName -Destination "${destination}" -Force
Copy-Item "${destination}" -Destination "${YAML}" -Force
Write-Output "cwagent-otel-collector config has been successfully fetched."
Write-Output "cwagent-otel-collector's configuration has been successfully fetched."
}
}

Expand Down