Code review comment for lp:~deeptik/linaro-license-protection/publish-to-snapshot

>
> 9
> parser.add_argument("-a", "--archive_info", dest="archive_info",
> 10
> help="Specify the job which resulted the archive to be
> stored.\
> 11
> Ex: ${JOB_NAME}/${BUILD_NUMBER} should be specified
> for \
> 12
> andriod and for \
> 13
> kernel-hwpack ${KERNEL_NAME}/${KERNEL_JOB_NAME}")
>
> This seems like confusing option. Reading the help, I don't have an idea why
> it is called "archive_info". Nor reading help I have clear picture what and
> how it should be used/works - that's too much specifics, out of context. This
> script is supposed to be called from Jenkins, and Jenkins cab basically pass
> to it it job name (in its native format) and build number. That's what script
> should accept, and help for options should describe just that, any specifics
> like the fact that job name have internal structure based on --job-type should
> be left to manual IMHO.

There is not archive_info anymore. This was an old change.

> Also, short option assignment is not ideal IMHO. For "job type", main word is
> (t)ype. For "kernel tree" it's (k)ernel (or (r)epository if should be
> generalized), "build number" is apparently (n)umber, which leaves "job name"
> to (j)ob.

Good suggestions. done.

> 60 + if args.job_type == "android":
> 61 + build_path = '/'.join([args.job_type, args.user_name,
> args.job_name])
>
> Well, nope, that's not correct. There's no "user name" on Jenkins level, only
> job name. That's what this script accepts. It then can reformat native Jenkins
> name into "external" name which we want to see.

Yes I agree that there is no username at jenkins level. But for android, I believe the jenkins-post-www.sh script line 38 refer to $TARGET_PATH/~$username/$jobname.
So I just tried to retain the extra work of separating the jobname into username + jobname.
Let me know if this is not required and what exactly did the username in the script meant to be.
I will make the changes accordingly.

> 71 + if not (os.path.isdir(uploads_path) or
> os.path.isdir(build_dir_path)):
> 72 + print "Missing build path", build_dir_path
> 73 + return FAIL
>
> The error message is incorrect - it checks 2 dirs, but reports problem as if
> applies to only one. Such cases are always great source of confusion and
> suffering (the latest one, (c) by Google:
> https://bugs.launchpad.net/bugs/949100)

I agree. Modified this to correct the error message.

> 79 + try:
> 80 + # Make a note of the contents of src dir so that
> 81 + # it can be used to validate the move to destination
> 82 + uploads_dirList = os.listdir(build_dir_path)
>
> That sounds complicated and overinsuring. Can't we make requirement that
> upload dir and destination dir were on the same FS, so we can just use mv?
> Even if we can't, why wouldn't we trust shutil.copy2()? It either does the
> copy, or throws an exception, so we can't miss failed move.
>

I did consider using the mv action for this. shutil.move has the following issue:
1) The destination dir should be non-existent. so in case of kernel hwpacks we intend to store all the hwpacks of one category in a single dir. so this move would fail. Even if we wanted to just move the files under the directory instead of the directory itself, the amount of code would be the same for copy and move.
2) move tends to internally use the shutil.copy2() when the source and the destination are not on the same FS. So , shutil.copy2() would be handy instead in case we moved source and destination on diff FS.

> Minor note - would be nice to use consistent var naming convention, like
> uploads_dir_list.
Done.

> 111 +def main():
> 112 + uploads_path = '/srv3/snapshots.linaro.org/uploads/'
> 113 + target_path = '/srv3/snapshots.linaro.org/www/'
> 114 + uploads_path = '/tmp/uploads/'
> 115 + target_path = '/tmp/www/'
>
> IMHO, it would be better to treat these as the constants, not as the
> variables, to be defined in the beginning of the script (like they were before
> r50).

Ok, noted.

I will submit the changes in sometime.

Thanks!!!
Deepti.