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

Update rag manifest that should fix the "manifest unknown" issue while image build #16

Merged
merged 1 commit into from
Oct 24, 2024

Conversation

mabulgu
Copy link

@mabulgu mabulgu commented Oct 24, 2024

Description

PR for updating rag manifest in the Containerfil. this should fix the "manifest unknown" issue with the push pipeline.

Type of change

  • Refactor
  • New feature
  • Bug fix
  • CVE fix
  • Optimization
  • Documentation Update
  • Configuration Update
  • Bump-up dependent library
  • Bump-up library or tool used for development (does not change the final image)
  • CI configuration change
  • Konflux configuration change

Related Tickets & Documents

  • Related Issue #
  • Closes #

Checklist before requesting a review

  • I have performed a self-review of my code.
  • PR has passed all pre-merge test jobs.
  • If it is a core feature, I have added thorough tests.

Testing

This can be tested after merging to main.

@mabulgu mabulgu changed the title Update rag manifest that should fix the "manifest unknown" Update rag manifest that should fix the "manifest unknown" issue while image build Oct 24, 2024
@mabulgu mabulgu requested a review from jameswnl October 24, 2024 19:11
@@ -1,6 +1,5 @@
# vim: set filetype=dockerfile
ARG LIGHTSPEED_RAG_CONTENT_IMAGE=quay.io/openshift-lightspeed/lightspeed-rag-content@sha256:8adbb6387204d39fb58eda8d7cdaab6407bb8e09d3050854588f89cd0af62077

ARG LIGHTSPEED_RAG_CONTENT_IMAGE=quay.io/openshift-lightspeed/lightspeed-rag-content@sha256:cbc9b0ea7c96fe400f2f8a8470976dd2c953c8e6239dc61b6aba2fad267363cd

Choose a reason for hiding this comment

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

This is a ARG. Please update it in the workflows, e.g. the push workflow is: https://github.com/ansible/ansible-chatbot-service/blob/main/.tekton/ansible-chatbot-service-push.yaml#L269

This ContainerFile will be a generic one on the upstream, we use the ARG to control what we need to use

Copy link
Author

Choose a reason for hiding this comment

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

yeah but only if we wanted to override the value with a custom one. in this case I am suspecting of a meta issue and by default this should be running without a problem.

What if someone wants to build the image without the pipeline? for the dev environmetn this should be sth already valid as well so I will insist on keeping the change. If this doesnt work, then I can revert it back to tthe upstream one as I will look at the problem from a different angle.

Choose a reason for hiding this comment

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

The default in the container file should be empty. And a local build can supply the value. Build ARG is part of the spec supported by both podman and docker.

That said I'll lgtm this for you

Copy link
Author

Choose a reason for hiding this comment

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

I agree. however my aim is keeping the changes in the container file as min as it needed. once we have these pipelines running we can focus on the other things I guess by improving the Containerfile and stuff around those.

@mabulgu mabulgu merged commit e3bf54e into main Oct 24, 2024
18 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.

2 participants