launchpad-reviewers team mailing list archive

[Jürgen Gmach <mp+480171@xxxxxxxxxxxxxxxxxx>]

I miss a descriptive commit message.

"Add environment variables for artifactory variables" is not telling the whole story.

Please try to come up with a description which really tells what your changes are doing.

Also, when you copy/paste most code from another MP (which is fine, no questions), it is also helpful to link to the other commit/MP to make the reviewers life easier.

I am a bit confused by the numbering used in the environment variables examples, see inline comment.

Diff comments:

> diff --git a/lib/lp/services/config/schema-lazr.conf b/lib/lp/services/config/schema-lazr.conf
> index f634d11..45d17e8 100644
> --- a/lib/lp/services/config/schema-lazr.conf
> +++ b/lib/lp/services/config/schema-lazr.conf
> @@ -301,6 +301,20 @@ secrets: none
>  scan_malware: False
>  
>  
> +[craftbuild.soss]
> +# value is a JSON Object
> +# example:
> +# environment_variables: {
> +#    "CARGO_ARTIFACTORY1_URL": "https://canonical.example.com/artifactory/api/cargo/cargo-upstream/index/";,
> +#    "CARGO_ARTIFACTORY1_READ_AUTH": "%(read_auth)s",
> +#    "CARGO_REGISTRY_GLOBAL_CREDENTIAL_PROVIDERS": "cargo:token",
> +#    "MAVEN_ARTIFACTORY1_URL": "https://canonical.example.com/artifactory/api/maven/maven-upstream/index";,
> +#    "MAVEN_ARTIFACTORY1_READ_AUTH": "%(read_auth)s",

What is the 1-suffix here for? Do we intend to have several ones with different numbers?  If there is a certain way we need to create variables, this needs to be documented.

> +# }
> +# 
> +environment_variables: none 
> +
> +
>  [codebrowse]
>  # Where to store codebrowse's sqlite "files changed" caches.  If
>  # empty, the logs will be stored in a folder called 'logs' next to the


-- 
https://code.launchpad.net/~ruinedyourlife/launchpad/+git/launchpad/+merge/480171
Your team Launchpad code reviewers is requested to review the proposed merge of ~ruinedyourlife/launchpad:artifactory-env-var-for-sourcecraft-builds into launchpad:master.