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

change the agent's image and packages for ARM platform #2814

Merged
merged 15 commits into from
Jul 25, 2023

Conversation

sharbuz
Copy link
Contributor

@sharbuz sharbuz commented Jul 24, 2023

What is the problem this PR solves?

the issue with ARM agent: https://buildkite.com/elastic/fleet-server-package-mbp/builds/213#_

How does this PR solve the problem?

use imagePrefix instead of image int the agent's config
add instanceType
add the checking of the OS type

How to test this PR locally

Related issues

#2517

@sharbuz sharbuz added the ci CI related tasks label Jul 24, 2023
@sharbuz sharbuz self-assigned this Jul 24, 2023
@sharbuz
Copy link
Contributor Author

sharbuz commented Jul 24, 2023

/package

@sharbuz
Copy link
Contributor Author

sharbuz commented Jul 24, 2023

/package

@sharbuz
Copy link
Contributor Author

sharbuz commented Jul 24, 2023

/package

@sharbuz
Copy link
Contributor Author

sharbuz commented Jul 25, 2023

/package

@sharbuz
Copy link
Contributor Author

sharbuz commented Jul 25, 2023

/package

@sharbuz
Copy link
Contributor Author

sharbuz commented Jul 25, 2023

/package

@sharbuz sharbuz changed the title change the agent's image for ARM change the agent's image and packages for ARM platform Jul 25, 2023
@sharbuz
Copy link
Contributor Author

sharbuz commented Jul 25, 2023

/package

@sharbuz
Copy link
Contributor Author

sharbuz commented Jul 25, 2023

/package

@sharbuz
Copy link
Contributor Author

sharbuz commented Jul 25, 2023

/package

@@ -21,7 +41,8 @@ add_bin_path() {
with_go() {
echo "Setting up the Go environment..."
create_workspace
retry 5 curl -sL -o ${WORKSPACE}/gvm "https://github.com/andrewkroh/gvm/releases/download/${SETUP_GVM_VERSION}/gvm-linux-amd64"
check_platform_architeture
retry 5 curl -sL -o ${WORKSPACE}/gvm "https://github.com/andrewkroh/gvm/releases/download/${SETUP_GVM_VERSION}/gvm-${platform_type,,}-${arch_type}"
Copy link
Contributor

@alexsapran alexsapran Jul 25, 2023

Choose a reason for hiding this comment

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

I think there is a typo, similar to the other places that you use the variable

Suggested change
retry 5 curl -sL -o ${WORKSPACE}/gvm "https://github.com/andrewkroh/gvm/releases/download/${SETUP_GVM_VERSION}/gvm-${platform_type,,}-${arch_type}"
retry 5 curl -sL -o ${WORKSPACE}/gvm "https://github.com/andrewkroh/gvm/releases/download/${SETUP_GVM_VERSION}/gvm-${platform_type}-${arch_type}"

Copy link
Contributor Author

@sharbuz sharbuz Jul 25, 2023

Choose a reason for hiding this comment

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

No, it doesn't a typo, this helps to transform the output of the variable ${platform_type} to lower case:
Example:

platform=$(uname)
echo "${platform}"
echo "${platform,,}"

output:

Linux
linux

@sharbuz sharbuz marked this pull request as ready for review July 25, 2023 10:00
@sharbuz sharbuz requested a review from a team as a code owner July 25, 2023 10:00
Comment on lines 55 to 56
local platform_type=$(uname)
local hw_type=$(uname -m)
Copy link
Contributor

Choose a reason for hiding this comment

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

These are defined at global level. Could those be used instead of getting again the values 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.

I thought about it, and I didn't find the correct answer. On one side these variables are used inside multiple functions, which means they are global, also, we shouldn't define these variables a lot of times. On another side, if we want to have self-sufficient functions we have to define them inside the functions. Where is the balance? WDYT @mrodm ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case, I would suggest using the global variables.

Copy link
Contributor

Choose a reason for hiding this comment

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

Currently I would also suggest to use global variables 👍

.buildkite/scripts/common.sh Outdated Show resolved Hide resolved
.buildkite/scripts/common.sh Outdated Show resolved Hide resolved
.buildkite/scripts/common.sh Outdated Show resolved Hide resolved
Co-authored-by: Mario Rodriguez Molins <[email protected]>
sharbuz and others added 3 commits July 25, 2023 14:36
Co-authored-by: Mario Rodriguez Molins <[email protected]>
Co-authored-by: Mario Rodriguez Molins <[email protected]>
@sharbuz
Copy link
Contributor Author

sharbuz commented Jul 25, 2023

/package

Copy link
Contributor

@mrodm mrodm left a comment

Choose a reason for hiding this comment

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

LGTM

@sharbuz sharbuz merged commit 586c96d into elastic:main Jul 25, 2023
6 checks passed
@sharbuz sharbuz deleted the change-agent-image-for-arm branch July 25, 2023 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci CI related tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants