npm preinstalled on macos for testing #5158

santipadilla · 2024-03-27T10:32:30Z

Related issue
#5128

Description

This PR implements a fix to the vulnerability test to have the option to pre-install npm on macOS machines.

Rebits

GJ. Some changes are required

Rebits · 2024-04-01T16:33:16Z

tests/end_to_end/test_vulnerability_detector/test_vulnerability_detector.py

@@ -109,6 +109,70 @@ def load_vulnerability_detector_configurations(host_manager):

 return configurations

+@pytest.fixture(scope='module')


This fixture will never be used in the test. Fixtures should be included in the desired tests or marked as autouse. Check the fixture documentation https://docs.pytest.org/en/6.2.x/fixture.html

Fixed here: 2126cdd

Rebits · 2024-04-01T16:33:40Z

tests/end_to_end/test_vulnerability_detector/test_vulnerability_detector.py

@@ -109,6 +109,70 @@ def load_vulnerability_detector_configurations(host_manager):

 return configurations

+@pytest.fixture(scope='module')
+def install_npm(host_manager: HostManager):
+ """Ensure npm is installed on macOS agents."""


This fixture not only ensures npm is installed but also performs the installation

Fixed here: 998c839

Rebits · 2024-04-01T16:53:38Z

tests/end_to_end/test_vulnerability_detector/test_vulnerability_detector.py

+ logger.info(f"Checking and installing Homebrew on {host}")
+ brew_check_command = "source /Users/vagrant/.zprofile && command -v brew"
+ brew_check_result = host_manager.get_host(host).ansible(
+ "shell",
+ brew_check_command,
+ become=True,
+ become_user='vagrant',
+ check=False
+ )
+ logger.info(f"Brew check result on {host}: {brew_check_result}")
+ # Install Homebrew if it is not already installed.
+ if brew_check_result['rc'] != 0:
+ logger.info("Installing Homebrew")
+ brew_install_command = 'NONINTERACTIVE=1 /bin/bash -c "$(curl -fsSL https://raw.githubusercontent.com/Homebrew/install/HEAD/install.sh)"'
+ install_result = host_manager.get_host(host).ansible("shell",
+ brew_install_command,
+ become=True,
+ become_user='vagrant',
+ check=False)
+ logger.info(f"Homebrew installation result on {host}: {install_result}")
+
+ add_brew_to_path = """
+ echo 'eval "$(/opt/homebrew/bin/brew shellenv)"' >> /Users/vagrant/.zprofile
+ eval "$(/opt/homebrew/bin/brew shellenv)"
+ """
+ path_result = host_manager.get_host(host).ansible("shell",
+ add_brew_to_path,
+ become=True,
+ become_user='vagrant',
+ check=False)
+ logger.info(f"Adding Homebrew to PATH result on {host}: {path_result}")
+ else:
+ logger.info("Homebrew is already installed.")
+ # Check if Node and npm is installed
+ logger.info(f"Checking and installing npm on {host}")
+ node_check_command = "PATH=/opt/homebrew/bin:$PATH && command -v node"
+ node_check_result = host_manager.get_host(host).ansible(
+ "shell",
+ node_check_command,
+ become=True,
+ become_user='vagrant',
+ check=False
+ )
+ logger.info(f"Node check result on {host}: {node_check_result}")
+ # Install node if it is not already installed.
+ if node_check_result['rc'] != 0:
+ logger.info("Installing Node.js and npm")
+ node_install_command = "PATH=/opt/homebrew/bin:$PATH && brew install node"
+ node_install_result = host_manager.get_host(host).ansible("shell",
+ node_install_command,
+ become=True,
+ become_user='vagrant',
+ check=False)
+ logger.info(f"Node.js and npm installation result on {host}: {node_install_result}")
+ else:
+ logger.info("Node.js and npm are already installed.")


Why not simplify the process by installing npm directly through its URL instead of using brew? This approach reduces complexity and minimizes the potential for errors in the block of code

Modified here: abb850f and check in this comment.

Rebits · 2024-04-01T16:56:02Z

tests/end_to_end/test_vulnerability_detector/test_vulnerability_detector.py

+ for host in host_manager.get_group_hosts('agent'):
+ os_type = host_manager.get_host_variables(host).get('os')
+
+ if os_type.startswith('macos'):


Let's enhance this by creating a variable that holds a list of operating systems for which npm will be installed. Then, let's incorporate that variable into this fixture. Additionally, let's utilize the 'macos' group instead of 'agent' to streamline the process and avoid unnecessary checks."

Modified here: abb850f and check in this comment.

Rebits

Include this change in the changelog

Rebits · 2024-04-04T07:50:19Z

tests/end_to_end/test_vulnerability_detector/test_vulnerability_detector.py

+ if node_check_result['rc'] != 0:
+ logger.info("Installing Node.js and npm")
+ # Download Node.js package
+ download_command = "curl -o /tmp/node-v21.7.1.pkg https://nodejs.org/dist/v21.7.1/node-v21.7.1.pkg"


Link should be included in a separate variable, defined at start of the module

Modified here: 220e419 and checked in this comment: #5128 (comment)

Rebits · 2024-04-04T07:51:21Z

tests/end_to_end/test_vulnerability_detector/test_vulnerability_detector.py

+ node_install_result = host_manager.get_host(host).ansible("shell",
+ node_install_command,
+ become=True,
+ become_user='vagrant',
+ check=False)
+ logger.info(f"Node.js and npm installation result on {host}: {node_install_result}")


The fixture does not warn in case of failure installing npm on macOS hosts

Rebits · 2024-04-04T07:55:31Z

tests/end_to_end/test_vulnerability_detector/test_vulnerability_detector.py

+ check=False)
+ logger.info(f"Node.js package download result on {host}: {download_result}")
+ # Install Node.js
+ node_install_command = "sudo installer -pkg /tmp/node-v21.7.1.pkg -target /"


Suggested change

node_install_command = "sudo installer -pkg /tmp/node-v21.7.1.pkg -target /"

node_install_command = "installer -pkg /tmp/node-v21.7.1.pkg -target /"

Modified here: 220e419 and checked in this comment: #5128 (comment)

Rebits · 2024-04-04T07:55:54Z

tests/end_to_end/test_vulnerability_detector/test_vulnerability_detector.py

+ download_result = host_manager.get_host(host).ansible("shell",
+ download_command,
+ become=True,
+ become_user='vagrant',
+ check=False)
+ logger.info(f"Node.js package download result on {host}: {download_result}")
+ # Install Node.js
+ node_install_command = "sudo installer -pkg /tmp/node-v21.7.1.pkg -target /"
+ node_install_result = host_manager.get_host(host).ansible("shell",
+ node_install_command,
+ become=True,
+ become_user='vagrant',
+ check=False)
+ logger.info(f"Node.js and npm installation result on {host}: {node_install_result}")


Any option to use install_package method instead?

Modified here: 220e419 and checked in this comment: #5128 (comment)

tests/end_to_end/test_vulnerability_detector/test_vulnerability_detector.py

+ for group in target_os_groups:
+ for host in host_manager.get_group_hosts(group):
+ # Check if Node and npm is installed
+ logger.info(f"Checking and installing npm on {host}")
+ node_check_command = "PATH=/usr/local/bin:$PATH && command -v node"
+ node_check_result = host_manager.get_host(host).ansible(
+ "shell",
+ node_check_command,
+ become=True,
+ become_user='vagrant',
+ check=False
+ )
+ logger.info(f"Node check result on {host}: {node_check_result}")
+ # Install node if it is not already installed.


…h-npm

Rebits

LGTM

…h-npm

npm preinstalled on macos for testing

2abdb1d

santipadilla marked this pull request as draft March 27, 2024 10:32

santipadilla self-assigned this Mar 27, 2024

santipadilla linked an issue Mar 27, 2024 that may be closed by this pull request

Create fixtures to provision macOS endpoints with npm dependencies. #5128

Closed

1 task

santipadilla mentioned this pull request Mar 27, 2024

Create fixtures to provision macOS endpoints with npm dependencies. #5128

Closed

1 task

santipadilla added 2 commits March 27, 2024 12:41

Condition if npm is installed

a1efcb8

homebrew for npm installation

8ec19ef

santipadilla marked this pull request as ready for review March 27, 2024 14:34

Rebits requested changes Apr 1, 2024

View reviewed changes

santipadilla marked this pull request as draft April 2, 2024 10:25

santipadilla added 4 commits April 2, 2024 12:29

autouse fix

2126cdd

Clarity comment

998c839

install npm from its url not from brew

abb850f

update changelog

05dc86e

santipadilla marked this pull request as ready for review April 2, 2024 12:31

Rebits requested changes Apr 4, 2024

View reviewed changes

santipadilla marked this pull request as draft April 4, 2024 10:37

santipadilla and others added 2 commits April 4, 2024 14:01

use of the install_package method

220e419

Merge branch 'enhancement/vd-e2e-tests' into 5128-provision-macos-wit…

cb6d9f9

…h-npm

santipadilla marked this pull request as ready for review April 4, 2024 12:03

Rebits approved these changes Apr 5, 2024

View reviewed changes

juliamagan and others added 2 commits April 5, 2024 10:47

Merge branch 'enhancement/vd-e2e-tests' into 5128-provision-macos-wit…

2938990

…h-npm

Merge branch 'enhancement/vd-e2e-tests' into 5128-provision-macos-wit…

13da807

…h-npm

davidjiglesias merged commit 8831b4b into enhancement/vd-e2e-tests Apr 5, 2024
0 of 2 checks passed

davidjiglesias deleted the 5128-provision-macos-with-npm branch April 5, 2024 08:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

npm preinstalled on macos for testing #5158

npm preinstalled on macos for testing #5158

santipadilla commented Mar 27, 2024

Rebits left a comment

Rebits Apr 1, 2024

santipadilla Apr 2, 2024

Rebits Apr 1, 2024 •

edited

Loading

santipadilla Apr 2, 2024

Rebits Apr 1, 2024 •

edited

Loading

santipadilla Apr 2, 2024

Rebits Apr 1, 2024 •

edited

Loading

santipadilla Apr 2, 2024

Rebits left a comment

Rebits Apr 4, 2024

santipadilla Apr 4, 2024

Rebits Apr 4, 2024

Rebits Apr 4, 2024

santipadilla Apr 4, 2024

Rebits Apr 4, 2024

santipadilla Apr 4, 2024

This comment was marked as resolved.

Rebits left a comment

		@@ -109,6 +109,70 @@ def load_vulnerability_detector_configurations(host_manager):

		return configurations

		@pytest.fixture(scope='module')

	node_install_command = "sudo installer -pkg /tmp/node-v21.7.1.pkg -target /"
	node_install_command = "installer -pkg /tmp/node-v21.7.1.pkg -target /"

npm preinstalled on macos for testing #5158

npm preinstalled on macos for testing #5158

Conversation

santipadilla commented Mar 27, 2024

Description

Rebits left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rebits Apr 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rebits Apr 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rebits Apr 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rebits left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Rebits left a comment

Choose a reason for hiding this comment

Rebits Apr 1, 2024 •

edited

Loading

Rebits Apr 1, 2024 •

edited

Loading

Rebits Apr 1, 2024 •

edited

Loading