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.

Sign up for GitHub

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

Jump to bottom

Add ansible role for Chroma #432

Merged
merged 20 commits into from
Oct 31, 2023
Merged

Add ansible role for Chroma #432

merged 20 commits into from
Oct 31, 2023

Conversation

TobiasDeBruijn
Copy link
Member

Requires some work still, in the systemd service template there are some TODO's left.

Should work besides that?

Siem2l
Siem2l suggested changes Mar 24, 2023
View reviewed changes
Copy link
Contributor

@Siem2l Siem2l left a comment

Choose a reason for hiding this comment

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

Minor comment but overall looks really good!

ansible/roles/chroma/tasks/main.yaml Outdated Show resolved Hide resolved
@TobiasDeBruijn
Copy link
Member Author

@JustNireon would you be able to take another look so we can merge this PR?

@Siem2l
Copy link
Contributor

Siem2l commented May 22, 2023

@JustNireon would you be able to take another look so we can merge this PR?

WIll do on Wednesday

@TobiasDeBruijn
Copy link
Member Author

@JustNireon Checking in, had any chance to take a look at this PR?

@Siem2l
Copy link
Contributor

Siem2l commented Jul 5, 2023

@TobiasDeBruijn Sorry have been quite busy recently. Will check this tomorrow but LGTM :)

@Mstiekema
Copy link
Member

Looks good! Later this week I'll deploy it to staging to check if everything is working. If that's okay we can merge to master :)

Only minor thing that we can still look at is the version variable @TobiasDeBruijn. Maybe use a tag for the latest/production build and clone/pull that one? Since we do not want to update the variable in this repo every time there is a new version out.

@TobiasDeBruijn
Copy link
Member Author

Should be able to do it, iirc GitHub has a releases/latest URL I can use. Will fix.

@TobiasDeBruijn
Copy link
Member Author

Actions seem to be broken.

@TobiasDeBruijn TobiasDeBruijn mentioned this pull request Aug 30, 2023
Merged
@TobiasDeBruijn
Copy link
Member Author

Looks good to merge I think.
Pxl migration on staging went well, I think. There are some performance concerns in Chroma itself yet to be resolved, but nothing stopping this from being merged or Chroma being production ready :)

@TobiasDeBruijn TobiasDeBruijn requested a review from Riscky October 10, 2023 08:32
Riscky
Riscky approved these changes Oct 10, 2023
View reviewed changes
Copy link
Member

@Riscky Riscky left a comment

Choose a reason for hiding this comment

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

Looks like this is ready, nice!
I've done some nitpicking, but after that those are fixed I think we can roll this out :)
Note that I haven't tested anything, as I don't have rights on the server anymore

ansible/roles/chroma/tasks/main.yaml Show resolved Hide resolved
ansible/roles/chroma/tasks/main.yaml Show resolved Hide resolved
ansible/roles/chroma/tasks/main.yaml Show resolved Hide resolved
ansible/roles/chroma/tasks/main.yaml Show resolved Hide resolved
ansible/roles/chroma/tasks/main.yaml Show resolved Hide resolved
ansible/roles/chroma/tasks/main.yaml Show resolved Hide resolved
ansible/roles/chroma/templates/chroma.service.j2 Outdated Show resolved Hide resolved
ansible/roles/chroma/templates/env.j2 Outdated Show resolved Hide resolved
ansible/roles/chroma/templates/env.j2 Outdated Show resolved Hide resolved
ansible/roles/packages/tasks/main.yml Outdated Show resolved Hide resolved
@TobiasDeBruijn
Copy link
Member Author

Good to merge I think

@Mstiekema
Copy link
Member

Docs URL doesn't seem to be working, please check

@TobiasDeBruijn
Copy link
Member Author

Fixed that one!

Mstiekema
Mstiekema approved these changes Oct 24, 2023
View reviewed changes
Copy link
Member

@Mstiekema Mstiekema left a comment

Choose a reason for hiding this comment

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

Looks like all requests for changes have been implemented, code itself looks good as well. Deploy to staging also seems to be working, LGTM! :shipit:

@SilasPeters SilasPeters added the enhancement Proposes (changes to) a feature label Oct 30, 2023
@TobiasDeBruijn
Copy link
Member Author

Awesome. Merge on Tuesday?

@TobiasDeBruijn TobiasDeBruijn merged commit 79800c6 into master Oct 31, 2023
1 check passed
@TobiasDeBruijn TobiasDeBruijn deleted the chroma branch October 31, 2023 19:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Riscky Riscky Riscky approved these changes

@Mstiekema Mstiekema Mstiekema approved these changes

@Siem2l Siem2l Awaiting requested review from Siem2l

@HugoPeters1024 HugoPeters1024 Awaiting requested review from HugoPeters1024

Assignees

@TobiasDeBruijn TobiasDeBruijn

Labels
enhancement Proposes (changes to) a feature review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@TobiasDeBruijn @Siem2l @Mstiekema @Riscky @HugoPeters1024 @SilasPeters