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

Had a couple of issues building and running this locally #5

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

oleorhagen
Copy link

Leaving this up here, not sure what is expected or not, so feel free to ignore.

@oleorhagen
Copy link
Author

@andreasfertig

@andreasfertig
Copy link
Owner

andreasfertig commented Apr 8, 2024

Hello @oleorhagen,

thanks for the PR! Can you please elaborate on what issues you had?

Andreas

@oleorhagen
Copy link
Author

Yes, of course.

Apologies, the PR did not come with a lot of information:

  1. The build fails, as the architecture arch in build.sh is lowercase, and hence not set properly.
  2. The naming of the container. It seems insights-test is expected. But it was currently named insights-testtest

@andreasfertig
Copy link
Owner

Hello @oleorhagen,

1. The build fails, as the architecture `arch` in `build.sh` is lowercase, and hence not set properly.

Correct. The required change seems to be the one in line 7. Adding TARGETARCH to docker build is unecessary. Docker takes care of that based on --platform linux/${ARCH}.

2. The naming of the container. It seems `insights-test` is expected. But it was currently named `insights-testtest`

insights-testtest is the correct name. I'm not sure which issues you hit here, but the tests in test.sh are based on that name.

I'm happy to merge the PR if you adjust it. In its current shape, the PR will break the C++ Insights web page or, hopefully, the GitHub Action build before that.

Andreas

@oleorhagen
Copy link
Author

Hello @oleorhagen,

1. The build fails, as the architecture `arch` in `build.sh` is lowercase, and hence not set properly.

Correct. The required change seems to be the one in line 7. Adding TARGETARCH to docker build is unecessary.

Alright, removed

Docker takes care of that based on --platform linux/${ARCH}.

2. The naming of the container. It seems `insights-test` is expected. But it was currently named `insights-testtest`

insights-testtest is the correct name. I'm not sure which issues you hit here, but the tests in test.sh are based on that name.

Ok, good :)

I've changed the name in the run.sh script to insights-test-test then

I'm happy to merge the PR if you adjust it. In its current shape, the PR will break the C++ Insights web page or, hopefully, the GitHub Action build before that.

Andreas

Also change the container image name in the runscript to `insights-test-test`,
since this is what is built in the test script.

Signed-off-by: Ole P. Orhagen <ole.petter.orhagen@mimiro.no>
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.

None yet

2 participants