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

SSE Hub #5286

Closed
wants to merge 6 commits into from
Closed

SSE Hub #5286

wants to merge 6 commits into from

Conversation

fschade
Copy link
Contributor

@fschade fschade commented Dec 23, 2022

Description

this PR is still in a early stage, needs more work, tests and features.
let's see where SSE takes us... a detailed description follows

Related Issue

Motivation and Context

How Has This Been Tested?

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:

Sorry, something went wrong.

@update-docs
Copy link

update-docs bot commented Dec 23, 2022

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@fschade fschade changed the title SSE for ocis SSE Hub Dec 23, 2022
@kobergj kobergj force-pushed the sse-notifications branch 2 times, most recently from aaae474 to c993bf9 Compare January 2, 2023 15:30
@ownclouders
Copy link
Contributor

ownclouders commented Jan 3, 2023

💥 Acceptance test localApiTests-apiSpacesShares-ocis failed. Further test are cancelled...

@mmattel
Copy link
Contributor

mmattel commented Jan 4, 2023

Note (important), info by @butonic (thanks):

  • a README.md needs to be created before merging
  • the new service needs to be added into the list of services in the makefile
  • checking if the new service creates the files necessary for documentation (adoc, yaml, md)
  • check if the descriptions/types/defaults of the envvars are all present and correct.
  • inform docs when the service is available so that we can create the service page in the admin docs.
    note: changes necessary on:
    create new hub service page,
    adapt deployment/services/tls.adoc
    adapt deployment/services/ports-used.adoc

services/hub/README.md Outdated Show resolved Hide resolved
@mmattel
Copy link
Contributor

mmattel commented Jan 12, 2023

Two things I identified:

  • Reading the change, following port is used for hub (9180). This needs an entry in the port-ranges file. Currently that port has following info: FREE (formerly used by accounts). Note that this file will publish to owncloud.dev automatically.
  • If not automagically created, we need a brief documentation for owncloud.dev for the hub service.

Sorry, something went wrong.

@mmattel
Copy link
Contributor

mmattel commented Jan 12, 2023

Regarding reame.md, how can one subscribe / unsubscribe to hub? Any example possible?

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 5 Code Smells

0.5% 0.5% Coverage
2.7% 2.7% Duplication

@kobergj kobergj force-pushed the sse-notifications branch from ab1da64 to 360c3e4 Compare April 3, 2023 10:40
@kobergj kobergj force-pushed the sse-notifications branch from 360c3e4 to 6bcdd34 Compare April 3, 2023 10:56
@kobergj
Copy link
Collaborator

kobergj commented Apr 3, 2023

Service checklist:

  • Provide a README.md for that service in the root folder of that service.
    • Use CamelCase for section headers.
  • For images and example files used in README.md:
    • Create a folder named md-sources on the same level where README.md is located. Put all the images and example files referenced by README.md into this folder.
    • Use absolute references like https://github.com/owncloud/ocis/blob/master/services/<service-name>/md-sources/file to make the content accessible for both README.md and owncloud.dev
  • If new CLI command are introduced, that command must be described in readme.md.
  • Add the service to the makefile in the ocis repo root.
  • Make the service startable for binary and individual startup:
    • For single binary add service to ocis/pkg/runtime
    • For individual startup add service to ocis/pkg/commands
  • Add the service to .drone.star to enable CI.
  • Inform doc team in an early stage to review the readme AND the environment variables created.
    • The description must reflect the behaviour AND usually has a positive code quality impact.
  • Create proper description strings for envvars - see other services for examples, especially when it comes to multiple values. This must include:
    • base description, set of available values, description of each value.
  • When suggestable commits are created for text changes and you agree, collect them to a batch and commit them. Do not forget to rebase locally to avoid overwriting the changes made.
  • If new envvars are introduced which serve the same purpose but in multiple services, an additional envvar must be added at the beginning of the list starting with OCIS_ (global envvar).
  • Ensure that a service has a debug port
  • If the new service introduces a new port:
  • Make sure to have a function FullDefaultConfig() in pkg/config/defaults/defaultconfig.go of your service. It is needed to create the documentation.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Apr 3, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 5 Code Smells

0.5% 0.5% Coverage
0.0% 0.0% Duplication

@kobergj kobergj mentioned this pull request Apr 3, 2023
@kobergj
Copy link
Collaborator

kobergj commented Apr 3, 2023

See #5998 for alternate approach

@fschade fschade closed this Jul 10, 2023
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

4 participants