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

WIP: create nomad-whisper state #746

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

WIP: create nomad-whisper state #746

wants to merge 15 commits into from

Conversation

rpurdel
Copy link
Contributor

@rpurdel rpurdel commented Dec 11, 2024

No description provided.

Copy link
Member

@aaronkvanmeerten aaronkvanmeerten left a comment

Choose a reason for hiding this comment

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

Should mostly all work, but the Jenkinsfile definitely should call our top level scripts/create-or-rotate rather than the terraform instance onfiguration.

sh """#!/bin/bash
export USER_PUBLIC_KEY_PATH=~/.ssh/ssh_key.pub
export ORACLE_GIT_BRANCH=\"$RELEASE_BRANCH\"
terraform/nomad-whisper/create-nomad-whisper-configuration.sh $SSH_USERNAME"""
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
terraform/nomad-whisper/create-nomad-whisper-configuration.sh $SSH_USERNAME"""
scripts/create-or-rotate-whisper.sh $SSH_USERNAME"""

unset SSH_USER

[ -z "$POOL_TYPE" ] && export POOL_TYPE="whisper"
[ -z "$ROLE" ] && export ROLE="whisper-pool"
Copy link
Member

Choose a reason for hiding this comment

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

Why not just make the role whisper? This value will show up in homer, among other places.

LOCAL_PATH=$(dirname "${BASH_SOURCE[0]}")

[ -z "$ROLE" ] && ROLE="nomad-pool"
[ -z "$POOL_TYPE" ] && POOL_TYPE="general"
Copy link
Member

Choose a reason for hiding this comment

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

this pool type should be whisper, yes? or really since we defined it above we can just remove these additions.

Suggested change
[ -z "$POOL_TYPE" ] && POOL_TYPE="general"


LOCAL_PATH=$(dirname "${BASH_SOURCE[0]}")

[ -z "$ROLE" ] && ROLE="nomad-pool"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
[ -z "$ROLE" ] && ROLE="nomad-pool"


[ -z "$ROLE" ] && ROLE="nomad-pool"
[ -z "$POOL_TYPE" ] && POOL_TYPE="general"
[ -z "$NAME" ] && NAME="$ENVIRONMENT-$ORACLE_REGION-$ROLE-$POOL_TYPE"
Copy link
Member

Choose a reason for hiding this comment

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

This name should be unique across environment, region and type. I don't believe we need both ROLE and POOL_TYPE in here. This was copied from the nomad generalized script, so it made more sense there than here.

[ -z "$S3_STATE_KEY" ] && S3_STATE_KEY="$ENVIRONMENT/nomad-whisper/$POOL_TYPE/terraform.tfstate"

[ -z "$BASE_IMAGE_TYPE" ] && BASE_IMAGE_TYPE="$NOMAD_BASE_IMAGE_TYPE"
[ -z "$BASE_IMAGE_TYPE" ] && BASE_IMAGE_TYPE="JammyBase"
Copy link
Member

Choose a reason for hiding this comment

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

I believe we want BASE_IMAGE_TYPE here as GPU, not JammyBase

Copy link
Member

Choose a reason for hiding this comment

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

oh, this is just a duplicate because we already define it above. on line 10

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.

2 participants