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

Feature/ZCS 10534 #1

Open
wants to merge 3 commits into
base: zimbra9/onlyoffice
Choose a base branch
from

Conversation

sreejan-chowdhury
Copy link

Changes :
Scripts to install rabbitmq, start and stop docservice/spellchecker/coverter;
https for docservice;
change in connection limit;
mysql as db
Updates from https://github.com/ONLYOFFICE/server.git

The branch has been created from tag : v6.3.1.10

The changes can be found in df31f2d

Steps taken :
Created a fork from onlyoffice/server.
Created a branch from v6.3.1.10.
Made changes in the branch.
Created new upstream for ZimbraOS/zm-onlyoffice
pushed new branch to ZimbraOS/zm-onlyoffice

To build on Ubuntu 14:
1.
git clone https://github.com/ONLYOFFICE/build_tools.git
cd build_tools/tools/linux

  1. Check : https://jira.corp.synacor.com/browse/ZCS-9616
    In: build_tools/scripts/base.py : def git_update(repo, is_no_errors=False, is_current_dir=False):
    Replace and Add the following:
if repo == "server":
     url = "<repo_address>"
else:
     url = "https://github.com/ONLYOFFICE/" + repo + ".git"
  1. python3 ./automate.py --branch= > build2.log 2>&1
  2. After the build is complete (more than 7 hrs) , the package can be found in
    ../../out/linux_64/onlyoffice/

Copy link

@desouzas desouzas left a comment

Choose a reason for hiding this comment

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

@sreejan-chowdhury Should you:

  • create a working tag base branch in this repo from v6.3.1.10
  • set this PR base branch to that working tag branch instead of master
  • once pr approved and merged: merge that working tag branch (which now includes these changes) to master
    • If the local changes were on top of the git history I would've suggested just rebasing master to latest stuff, then cherry-picking your commits to keep local changes in this repo on top

Less changes for reading.

For reviewers these appear to be local changes:
https://github.com/ONLYOFFICE/server/compare/v6.3.1.10...sreejan-chowdhury:feature/ZCS-10534?expand=1

My comments are not a full review, just casual glance.

@@ -852,7 +908,9 @@ const c_oPublishType = {
meta: 11,
forceSave: 12,
closeConnection: 13,
changesNotify: 14
changesNotify: 14,
changeConnecitonInfo: 15,

Choose a reason for hiding this comment

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

typo (this appears to be from the tag, and not you)

Copy link
Author

Choose a reason for hiding this comment

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

This is from the tag

@@ -32,28 +32,18 @@

'use strict';

const config = require('config');
const configL = config.get('license');

Choose a reason for hiding this comment

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

sketchy file changes 😂 (this appears to be from the tag, and not you)

Copy link
Author

Choose a reason for hiding this comment

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

This is from the tag

@@ -93,9 +93,9 @@ if (cluster.isMaster) {
if (config.has('ssl')) {
const privateKey = fs.readFileSync(config.get('ssl.key')).toString();
const certificateKey = fs.readFileSync(config.get('ssl.cert')).toString();
const trustedCertificate = fs.readFileSync(config.get('ssl.ca')).toString();
//const trustedCertificate = fs.readFileSync(config.get('ssl.ca')).toString();

Choose a reason for hiding this comment

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

Did you intend to remove this commented line?

Copy link
Author

Choose a reason for hiding this comment

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

Left it by mistake. Thanks

bin/configure.py Outdated
data = json.load(f)

while True:
docservice_port = input("Enter docservice port number (Press enter to keep default): ")
Copy link

@desouzas desouzas May 26, 2021

Choose a reason for hiding this comment

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

Would it be more helpful to show the actual default value (e.g. [default here]) instead of just mentioning there is one? Same for the other prompts in this file.

Copy link
Author

Choose a reason for hiding this comment

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

This file will no longer be needed.

Copy link

@thefoolwhocodes thefoolwhocodes left a comment

Choose a reason for hiding this comment

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

Better way to do it:
Create a branch from ZimbraOS/zm-onlyoffice tag.
Raise PR against it for better visiblity.

@sreejan-chowdhury sreejan-chowdhury changed the base branch from master to zimbra9/onlyoffice July 8, 2021 17:57
@sreejan-chowdhury
Copy link
Author

Better way to do it:
Create a branch from ZimbraOS/zm-onlyoffice tag.
Raise PR against it for better visiblity.

A new base branch from tag v6.3.1.10 has been created for the ease of seeing the changes made by me.

@adriangibanelbtactic
Copy link

  content=$(${zmjq} --arg server_crt "$server_crt" '.services.CoAuthoring.ssl.cert=$server_crt' ${defaultjson_path})
  echo "${content}" > ${defaultjson_path}
  content=$(${zmjq} --arg server_crt "$server_crt" '.SpellChecker.ssl.cert=$server_crt' ${defaultjson_path})
  echo "${content}" > ${defaultjson_path}
  content=$(${zmjq} --arg server_key "$server_key" '.services.CoAuthoring.ssl.key=$server_key' ${defaultjson_path})
  echo "${content}" > ${defaultjson_path}
  content=$(${zmjq} --arg server_key "$server_key" '.SpellChecker.ssl.key=$server_key' ${defaultjson_path})
echo "${content}" > ${defaultjson_path}

Similar code is done several times at this new zmonlyofficeconfig file.

Check:

content=$(${zmjq} '.services.CoAuthoring.token.enable.browser=true' ${defaultjson_path})
echo "${content}" > ${defaultjson_path}
content=$(${zmjq} '.services.CoAuthoring.token.enable.request.inbox=true' ${defaultjson_path})
echo "${content}" > ${defaultjson_path}
content=$(${zmjq} '.services.CoAuthoring.token.enable.request.outbox=true' ${defaultjson_path})
echo "${content}" > ${defaultjson_path}
content=$(${zmjq} '.services.CoAuthoring.requestDefaults.rejectUnauthorized=false' ${defaultjson_path})
echo "${content}" > ${defaultjson_path}
.

This is overwriting the same file again with the latest value ( > ) instead of appending values ( >> ), isn't it ?

You should end up with some configuration file which do not have the needed keys for this to work properly.

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.

4 participants