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

fix the docker-compose wopi #8483

Merged
merged 1 commit into from
Apr 26, 2024
Merged

fix the docker-compose wopi #8483

merged 1 commit into from
Apr 26, 2024

Conversation

2403905
Copy link
Contributor

@2403905 2403905 commented Feb 19, 2024

Description

Related Issue

Motivation and Context

How Has This Been Tested?

  • test environment:
  • test case 1:
  • test case 2:
  • ...

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

Copy link

sonarcloud bot commented Feb 19, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

micbar
micbar previously requested changes Feb 19, 2024
Copy link
Contributor

@micbar micbar left a comment

Choose a reason for hiding this comment

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

I would rather try to find a solution wihout the curl workaround in the entrypoint override.

This example is used by community for production deployments.

Can we use the compose depends_on logic here?

@2403905
Copy link
Contributor Author

2403905 commented Feb 19, 2024

@micbar
The depends_on will work correctly if we use it with the condition service_healthy and in this case we have to perform the healthcheck somehow in a collabora and onlyoffice.
https://docs.docker.com/compose/compose-file/compose-file-v3/#healthcheck

services:
  ocis-appprovider-collabora:
    image: ocis
    depends_on:
      collabora:
        condition: service_healthy
        
  collabora:
    image: collabora
    healthcheck:
      test: ["CMD", "curl", "-f", "http://localhost"]
      interval: 1s
      timeout: 3s
      retries: 30

upd: https://sdk.collaboraonline.com/docs/installation/CODE_Docker_image.html#troubleshooting

@2403905
Copy link
Contributor Author

2403905 commented Feb 19, 2024

So in both cases, we must add a curl in the app provider or in the office client container. I would prefer not to change the office client containers.

@2403905 2403905 requested a review from micbar February 19, 2024 09:54
@wkloucek
Copy link
Contributor

Is this a duplicate of #8144 ?

Instead of working around it, we should properly fix it: #3832 (comment)

@2403905
Copy link
Contributor Author

2403905 commented Feb 20, 2024

Looks like it is a duplicate of #8144

@butonic
Copy link
Member

butonic commented Feb 21, 2024

how do we move forward?

@kulmann kulmann requested a review from wkloucek April 4, 2024 09:23
@wkloucek
Copy link
Contributor

wkloucek commented Apr 4, 2024

@kulmann I already reviewed it:

Instead of working around it, we should properly fix it: #3832 (comment)

@wkloucek wkloucek removed their request for review April 4, 2024 09:40
@2403905 2403905 enabled auto-merge (rebase) April 25, 2024 10:22
@DeepDiver1975
Copy link
Member

Tested ✔️ and collabora shows up in the file action menu right away and it can be opened.
But it cries about no disk space .... but I guess this is unrelated
image

@2403905
Copy link
Contributor Author

2403905 commented Apr 25, 2024

It works well for me!
Screenshot 2024-04-25 at 18 14 01

Copy link

sonarcloud bot commented Apr 25, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@DeepDiver1975
Copy link
Member

tested again after rebase - collabora is shown in the file action menu right away.
but i still get the disk space issue - but this is a different ropic.

let's merge this ...

@DeepDiver1975 DeepDiver1975 dismissed micbar’s stale review April 26, 2024 07:24

The same workaround has already been applied to OnlyOffice as well.

@2403905 2403905 merged commit 76f97ba into owncloud:master Apr 26, 2024
2 checks passed
@micbar
Copy link
Contributor

micbar commented Apr 26, 2024

No!
I was against merging that.

You put user:0 on the container which is a total no go.

Please revert.

@DeepDiver1975
Copy link
Member

DeepDiver1975 commented Apr 26, 2024

but then you need to revert onlyoffice as well and the whole example is useless.

introduced 3 years ago - https://github.com/owncloud/ocis/blame/76f97ba0c430f1fd5f0465bf78149149d27c468d/deployments/examples/ocis_wopi/docker-compose.yml#L148

@micbar

@wkloucek
Copy link
Contributor

wkloucek commented Apr 26, 2024

Which brings us back to #3832

But there is also another workaround:

healthcheck:
  test:
    - CMD
    - /bin/sh
    - -c
    - curl --silent -- fail http://ocis:9200/app/list | grep '"name":"Collabora"'

It similar to the workaround in the oCIS Helm Chart: https://github.com/owncloud/ocis-charts/blob/ca7f57a448adfd2aed3c28e1b0c0146d226547ae/charts/ocis/templates/appprovider/deployment.yaml#L88-L93

This actually also "works" for the case that you're restarting the appregistry.

@DeepDiver1975
Copy link
Member

#8984

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.

Collabora is not available
5 participants