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

Mirroring scripts #1975

Merged
merged 12 commits into from
Jan 17, 2018
Merged

Mirroring scripts #1975

merged 12 commits into from
Jan 17, 2018

Conversation

vladak
Copy link
Member

@vladak vladak commented Jan 11, 2018

This set of changes adds a bunch of Python scripts to synchronize SCM repositories (git, Mercurial, CVS, SVN, Teamware are supported) with their parents/upstream. Plus pre-requisite changes to Messages.

The main mirror.py script is supposed to be run per-project via the already existing sync.py script.

echo " repository:"
echo " - repository paths relative to source root are specified as <tags>"
echo " - command is specified in message <text>:"
echo " - \"get-repo-type\" - returns type of the repository or \"N/A\""
Copy link
Contributor

Choose a reason for hiding this comment

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

Update the main README as well

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@tarzanek tarzanek added this to the 1.1 milestone Jan 12, 2018
@vladak
Copy link
Member Author

vladak commented Jan 16, 2018

I found a Pythonista in the company to do additional (sic!) review for the python scripts, still need someone to look at the Messages changes. Also, I will probably switch our production instance to these scripts to give it additional testing.

* @return string representation of the field value
* @throws java.io.IOException
*/
protected String invokeGetter(Configuration config, String field) throws IOException {
Copy link
Contributor

Choose a reason for hiding this comment

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

this may be a candidate for a util function along with invokeSetter

Copy link
Member Author

Choose a reason for hiding this comment

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

you mean moving this to org.opensolaris.opengrok.util as a utility class ? or possibly to Configuration.java ?

Copy link
Contributor

Choose a reason for hiding this comment

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

t thought more about a new utility class but you decide both is better than have it here

Copy link
Contributor

Choose a reason for hiding this comment

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

or maybe if you extract this to the ClassUtils and make it more generic (for any object and not just for configuration) and then you can delegate these calls from here. How about that?

Copy link
Member Author

Choose a reason for hiding this comment

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

yep, that's what I did.

@@ -17,7 +17,7 @@
* CDDL HEADER END
*/

/*
/*
* Copyright (c) 2017, 2017, Oracle and/or its affiliates. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

copyright

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed, hopefully we won't have to deal with this anymore soon.

@@ -252,6 +301,7 @@ public void validate() throws Exception {
if (toInteger(hasTag("setconf"))
+ toInteger(hasTag("getconf"))
+ toInteger(hasTag("auth"))
+ toInteger(hasTag("get"))
+ toInteger(hasTag("set")) > 1) {
throw new Exception("The message tag must be either setconf, getconf, auth or set");
Copy link
Contributor

@tulinkry tulinkry Jan 16, 2018

Choose a reason for hiding this comment

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

+ get

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed by rewriting this test

@@ -17,7 +17,7 @@
* CDDL HEADER END
*/

/*
/*
* Copyright (c) 2017, Oracle and/or its affiliates. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

copy

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed


switch (msgtext) {
case "get-repo-type":
List<String> types = new ArrayList<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe a linked list? or array list with known capacity? to reduce the unnecessary overhead

Copy link
Member Author

Choose a reason for hiding this comment

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

used specified initial capacity

types.add("N/A");
}
}
ret = types.stream().collect(Collectors.joining("\n")).toString();
Copy link
Contributor

Choose a reason for hiding this comment

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

just some thoughts..

this gives for a message with tags
-t opengrok -t opengrok-master -t nothing
the result

git
git
N/A

isn't something like

opengrok: git
opengrok-master: git
nothing: N/A

better?
or some json?

Copy link
Member Author

Choose a reason for hiding this comment

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

okay, will use the colon separated output.

I will save JSON output for #1801.

@Override
public void validate() throws Exception {
String command = getText();
Set<String> allowedText = new TreeSet<>(Arrays.asList("get-repo-type"));
Copy link
Contributor

Choose a reason for hiding this comment

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

allowedTexts

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

@@ -17,7 +17,7 @@
* CDDL HEADER END
*/

/*
/*
* Copyright (c) 2017, Oracle and/or its affiliates. All rights reserved.
* Portions Copyright (c) 2017, Chris Fraire <cfraire@me.com>.
Copy link
Contributor

Choose a reason for hiding this comment

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

18

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

public void setUp() throws IOException {
Assume.assumeTrue(new MercurialRepository().isWorking());
Assume.assumeTrue(new SubversionRepository().isWorking());
Assume.assumeTrue(new GitRepository().isWorking());
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I included the repeateable conditional rule for this, can you try it?

Copy link
Member Author

Choose a reason for hiding this comment

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

these are not needed, removed.

Assume.assumeTrue(new MercurialRepository().isWorking());
Assume.assumeTrue(new SubversionRepository().isWorking());
Assume.assumeTrue(new GitRepository().isWorking());

Copy link
Contributor

Choose a reason for hiding this comment

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

do you need git and subversion in this test?

Copy link
Member Author

Choose a reason for hiding this comment

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

technically only the mercurial is needed. I have extended the test to check git repo as well and used the repeated annotation.

@vladak
Copy link
Member Author

vladak commented Jan 16, 2018

For the record, here are the python comments:

(11:25:01) timf: vladimir.kotal: platform/solaris/ips/create.sh looks pretty antiquated. You really should replace that with a single p5m manifest and pkgsend publish
(11:25:15) timf: rather than publishing via repeated pkgsend add actions
(11:37:09) timf: tools/sync/hook.py looks a bit weird too. There's a 'cwd' parameter for popen call, so you're kinda going the long way around here.
(11:37:46) jan.parcel left the room.
(11:41:13) timf: tools/sync/mirror.py line 140, no need for the '+' at the end of the line
(11:41:28) timf: string literals continued across lines are automatically concatenated.
(11:42:17) timf: line 151, and elsewhere I'd use double quotes rather than single quotes - generally, just pick a style and stick with it.
(11:43:25) timf: line 184, no spaces between the name, the equals sign and the value. eg. good: parm_name=param_value, bad: param_name = param_value
(11:45:18) timf: line 237 should probably be log.error rather than log.debug
(11:49:38) timf: tools/sync/teamware.py a bit of extra safety might be to verify that os.path.isdir(command) before proceeding.
(11:50:19) timf: line 43, copy/paste coding s/hg_command/bringover_command/
(11:53:48) timf: tools/sync/utils.py def which(..) can be replaced by shutil.which(..) if you're guaranteed to have python3.
(11:53:48) timf: >>> import shutil

shutil.which("cc")
'/opt/solarisstudio12.4/bin/cc'

(11:58:28) timf: line 28, os.mkdir should perhaps be os.makedirs, so as to make any intermediate dirs along the way
(12:00:42) timf: the get_dict_val function seems a bit weird, not wrong, just.. odd. (you know you can do dictionary.get(key) and it'll return None if the key doesn't exist, or dictionary.get(key, default) and it'll return default if key doesn't exist)
(12:00:55) timf: ok, I'm done. Generally, nothing too weird there.

@tarzanek
Copy link
Contributor

looking forward to try it out once merged, thank you Vlad :)

@vladak
Copy link
Member Author

vladak commented Jan 16, 2018

@tarzanek I am a bit worried about the Python scripts having hardcoded Python 3.4 so might not run on anything else (3.5) even if it could.

@tarzanek
Copy link
Contributor

d'oh ... so once again the history repeats and the same language is not compatible with its own old versions?

@vladak
Copy link
Member Author

vladak commented Jan 17, 2018 via email

@tulinkry
Copy link
Contributor

how about /usr/bin/env python3 ? this should lead to the recent version of python3 on the system, or?

@vladak
Copy link
Member Author

vladak commented Jan 17, 2018

I was thinking about python3 however that would not work on Solaris. And the most recent version guarantee does not universally hold true.

@tulinkry
Copy link
Contributor

you can add a check in the code that the version is atleast 3 and throw an exception?

@vladak
Copy link
Member Author

vladak commented Jan 17, 2018

The version check is already present however this is still not going to work on a system which has both Python 2 and 3 but /usr/bin/python points to Python 2.

@tulinkry
Copy link
Contributor

Ok I didn't see it

@vladak
Copy link
Member Author

vladak commented Jan 17, 2018

I will just bite the bullet and use python3 at the expense of having to create python3 symlink on systems that do not have it.

@tarzanek
Copy link
Contributor

I agree, since it seems most OSes have python3 symlink, no ?

thnx
L

@vladak
Copy link
Member Author

vladak commented Jan 17, 2018

  • Ubuntu has the symlink
  • macOS has the symlink if Python 3.x was installed from Homebrew
  • Solaris does not have it

@tulinkry
Copy link
Contributor

yes I am for python3

@tarzanek
Copy link
Contributor

tarzanek commented Jan 17, 2018

windows also doesn't have it, but I am OK with Ubuntu(Debian) and macOS :)
(and I think RHEL/Centos have it too)

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