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

Perforce: resolve #301 #378

Merged
merged 9 commits into from
Nov 28, 2024
Merged

Perforce: resolve #301 #378

merged 9 commits into from
Nov 28, 2024

Conversation

GrzesiekO
Copy link
Contributor

@GrzesiekO GrzesiekO commented Oct 31, 2024

Issue number:
#301

Summary

This PR fixes and improves a few things:

  1. Added support for setting Unicode support in Perforce during configuration (Feature request: Support setting Unicode mode during p4_configure.sh run #301)
  2. The main Terraform module has been updated to allow adding existing security groups without creating a default one. This fixes an issue where the module was creating a new security group even when one was already provided.
  3. Various fixes in p4_configure.sh and improvements to the reliability of the setup.

Changes

Minor fixes in p4_configure.sh
New variables in the p4_configure.sh script: --unicode and --selinux. These are also set by Terraform in the EC2 userdata that invokes the p4_configure.sh script. In Terraform, they both default to "false"

Please provide a summary of what's being changed

User experience

New variable in Terraform for helix-core module "unicode" to address unicode mode in Helix Core.
New variable in Terraform for helix-core module "selinux" to make updating SElinux labels optional depending on underlying OS used.
Both default to false.

Please share what the user experience looks like before and after this change

Checklist

If your change doesn't seem to apply, please leave them unchecked.

  • [ x] I have performed a self-review of this change
  • [ x] Changes have been tested
  • [ x] Changes are documented
Is this a breaking change? No it is not a breaking change it fixes minor bugs and adds one core requested funcitonality

Acknowledgment

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Disclaimer: We value your time and bandwidth. As such, any pull requests created might not be successful.

@GrzesiekO GrzesiekO requested a review from a team as a code owner October 31, 2024 14:39
@GrzesiekO GrzesiekO requested a review from kylesomers October 31, 2024 14:39
henrykie
henrykie previously approved these changes Oct 31, 2024
Copy link
Contributor

@henrykie henrykie left a comment

Choose a reason for hiding this comment

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

lgtm

assets/packer/perforce/helix-core/p4_configure.sh Outdated Show resolved Hide resolved
jorisdon
jorisdon previously approved these changes Oct 31, 2024
@henrykie
Copy link
Contributor

@GrzesiekO please rebase the base branch onto this feature branch, and potentially consider renaming / squashing your second commit so that both use conventional commit structure.

@henrykie henrykie removed the request for review from kylesomers October 31, 2024 15:12
@henrykie
Copy link
Contributor

henrykie commented Nov 7, 2024

@GrzesiekO You want to update and merge?

@GrzesiekO GrzesiekO dismissed stale reviews from jorisdon and henrykie via e8575cd November 7, 2024 17:10
@jorisdon jorisdon force-pushed the ochmang-helixcore-improvments branch 4 times, most recently from 56fe076 to acabc6b Compare November 13, 2024 16:37
jorisdon
jorisdon previously approved these changes Nov 13, 2024
@jorisdon jorisdon changed the title Ochmang helixcore improvments Perforce: resolve #301, fix #321, and perform installation through Ansible Nov 13, 2024
@jorisdon jorisdon changed the title Perforce: resolve #301, fix #321, and perform installation through Ansible Perforce: resolve #301, and perform installation through Ansible Nov 13, 2024
Copy link
Contributor

@henrykie henrykie left a comment

Choose a reason for hiding this comment

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

Please respond to comments. We do not need to fix these issues immediately, but should track them.

@jorisdon jorisdon self-requested a review November 15, 2024 14:47
@henrykie henrykie self-requested a review November 15, 2024 14:47
henrykie
henrykie previously approved these changes Nov 15, 2024
@jorisdon
Copy link
Contributor

I am not entirely sure how this Ansible playbook and the Packer template are supposed to be used. Do we expect users to build the Perforce AMI via Packer (which loads p4_configure.sh onto the AMI) and then later on install Perforce Helix Core through the Ansible playbook?

@henrykie
Copy link
Contributor

+1. I was under the impression we were eventually replacing p4_configure.sh with configure_helix_core.playbook.yml or something like that. Then we would just invoke it on startup via an SSM association instead of using user data...

@GrzesiekO GrzesiekO changed the title Perforce: resolve #301, and perform installation through Ansible Perforce: resolve #301 Nov 28, 2024
Copy link
Contributor

@jorisdon jorisdon left a comment

Choose a reason for hiding this comment

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

Contents LGTM, though we should squash and merge this PR or clean up commit history for this (I don't recommend using rebase and merge at the moment)

@jorisdon
Copy link
Contributor

To clarify on the answer to my previous question:

I am not entirely sure how this Ansible playbook and the Packer template are supposed to be used. Do we expect users to build the Perforce AMI via Packer (which loads p4_configure.sh onto the AMI) and then later on install Perforce Helix Core through the Ansible playbook?

The Ansible playbook was incomplete / not fully usable yet, and we've decided to remove it to allow the pull request to proceed.

@henrykie henrykie self-requested a review November 28, 2024 17:55
GrzesiekO and others added 9 commits November 28, 2024 17:58
…ting security groups

The main Terraform module has been updated to allow adding existing security groups without creating a default one. This fixes an issue where the module was creating a new security group even when one was already provided.

Additionally, the core module has been enhanced to provide Unicode support, enabling expanded character set handling.
Other minor fixes and cleanup.

chore: fix typo
- Replace p4_configure.sh and p4_setup.sh with Ansible playbook
- Eliminate need for Packer-built AMI and automates Amazon Linux 2023
- Add tasks for downloading and configuring Helix binaries
- Implement platform-specific binary selection
- Ensure correct placement of binaries in /hxdepots/sdp/helix_binaries
- Add error handling and retries for binary downloads
- Improve automation and consistency in Perforce Helix Core setup

This playbook automates the Perforce Helix Core installation and
configuration process, providing a more robust and maintainable
solution compared to the previous shell scripts. It dynamically
selects the appropriate binaries based on the target system's
architecture, enhancing cross-platform compatibility.

This also adds an SSM document for Helix Core configuration

- Create new SSM module for applying Ansible playbooks
- Update p4_configure_playbook.yml with improved installation steps
- Modify p4_configure.sh remove unecessary selinux commands

This commit introduces an SSM document to streamline the deployment of
Perforce Helix Core on EC2 instances using Ansible playbooks. The
configuration process is now more automated and consistent across
deployments.
chore: remove unused file

Deleted ssm_module.tf as it was no longer needed in the project.
This file removal doesn't affect any functionality.
@henrykie henrykie force-pushed the ochmang-helixcore-improvments branch from 27028b9 to d6c6d8f Compare November 28, 2024 17:58
@henrykie henrykie merged commit 6f17a39 into main Nov 28, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants