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

Add kubernetes tentacle docker build #699

Merged
merged 7 commits into from
Nov 29, 2023

Conversation

APErebus
Copy link
Contributor

@APErebus APErebus commented Nov 28, 2023

Background

The Kubernetes Tentacle can only be distributed via docker container and has a few new environment variables/configuration points than the original tentacle container. This PR adds a new Dockerfile for setup scripts

Results

I just copied the linux dockerfile + install scripts and starting stripping out things that aren't relevant.

The compose build tags the container as octopusdeploy/kubernetes-tentacle

Shortcut story: [sc-60398]

How to review this PR

Quality ✔️

Pre-requisites

  • I have read How we use GitHub Issues for help deciding when and where it's appropriate to make an issue.
  • I have considered informing or consulting the right people, according to the ownership map.
  • I have considered appropriate testing for my change.

@APErebus APErebus requested a review from a team as a code owner November 28, 2023 03:04
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 was able to remove a bunch of stuff relating to including Docker into this container, which is not supported

Comment on lines +44 to +48
ENV OCTOPUS__K8STENTACLE__NAMESPACE=""
ENV OCTOPUS__K8STENTACLE__USEJOBS="True"
ENV OCTOPUS__K8STENTACLE__JOBSERVICEACCOUNTNAME=""
ENV OCTOPUS__K8STENTACLE__JOBVOLUMEYAML=""
Copy link
Contributor Author

@APErebus APErebus Nov 28, 2023

Choose a reason for hiding this comment

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

k8s specific environment variables defined in #690

fi
}

function validateVariables() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because I know we are running as a kubernetes tentacle, I was able to clean up a lot of the validation here

Comment on lines +122 to +127
if [[ ! -z "$TargetEnvironment" ]]; then
IFS=',' read -ra ENVIRONMENTS <<<"$TargetEnvironment"
for i in "${ENVIRONMENTS[@]}"; do
ARGS+=('--environment' "$i")
done
fi
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do support multiple environments/tenants via a command separated list in the environment variable in the container.

Copy link

This pull request has been linked to Shortcut Story #60398: Create new container image for Agent.

@@ -0,0 +1,60 @@
FROM mcr.microsoft.com/dotnet/runtime-deps:6.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Once we get things running, I'd love to see if 6.0-alpine works...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, it probably will, the only trick is that it doesn't have bash installed which the install script needs 😁

Copy link
Contributor Author

@APErebus APErebus Nov 28, 2023

Choose a reason for hiding this comment

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

or, as @eddymoulton said, the new chiseled images for ubuntu could be good too

@APErebus APErebus force-pushed the ap/new-kubernetes-tentacle-container branch from 6700fd9 to b83c702 Compare November 28, 2023 03:58
@APErebus APErebus force-pushed the ap/new-kubernetes-tentacle-container branch from c8833b9 to 8572100 Compare November 28, 2023 23:03
@APErebus APErebus changed the base branch from ap/k8s-jobs-script-execution to main November 28, 2023 23:03
Copy link
Contributor

@MissedTheMark MissedTheMark left a comment

Choose a reason for hiding this comment

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

Looks good to me. We'll find out quick enough once we start using this ourselves whether or not anything is missing 😁
Just left one comment on the environment/role missing error message.

Comment on lines 47 to 55
if [[ -z "$TargetEnvironment" ]]; then
echo "Please specify an environment name with the 'TargetEnvironment' environment variable" >&2
exit 1
fi

if [[ -z "$TargetRole" ]]; then
echo "Please specify a role name with the 'TargetRole' environment variable" >&2
exit 1
fi
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these error messages specify "one or more"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, good point

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in fa0079a

@APErebus APErebus enabled auto-merge (squash) November 29, 2023 06:35
@APErebus APErebus merged commit 3f84d37 into main Nov 29, 2023
46 checks passed
@APErebus APErebus deleted the ap/new-kubernetes-tentacle-container branch November 29, 2023 09:26
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