launchpad-reviewers team mailing list archive

[Ines Almeida <mp+475344@xxxxxxxxxxxxxxxxxx>]

Review: Needs Information

Thanks for the MP!
The biggest show stopper for me is the fetch_service_snap_channel change to `candidate` - this doesn't feel right.

Diff comments:

> diff --git a/lp-fetch-service/bundle.yaml b/lp-fetch-service/bundle.yaml
> index e764a4a..6514f9a 100644
> --- a/lp-fetch-service/bundle.yaml
> +++ b/lp-fetch-service/bundle.yaml
> @@ -1,6 +1,6 @@
>  {%- if stage_name == "production" %}
>  {%-   set devel = False %}
> -{%-   set fetch_service_snap_channel = "latest/edge" %}
> +{%-   set fetch_service_snap_channel = "latest/candidate" %}

What's the reasoning for the fetch_service channel change to candidate instead of edge? That doesn't feel right

candidate is the channel we have for qastaging for testing. If we want the version from candidate in production, we should ask Claudio to promote it to edge

>  {%-   set nagios_context = "lp-prodstack-fetch-service" %}
>  {%-   set nagios_hostgroups = "prodstack-lp" %}
>  {%-   set nagios_master = "nagios.ps5.internal" %}
> @@ -20,15 +20,97 @@
>  {%-   set nagios_master = "localhost" %}
>  {%- endif -%}
>  
> +{%- macro inspectors_git_configuration() %}

I don't see a reason to have this be a macro given it's static - perhaps it can just be list variables similar to the ones set at the top of the page?

> +{%- if stage_name == "production" %}
> +- https://git.pkg.store:443/**
> +- https://git.staging.snapcraftcontent.com:443/**
> +- https://git.launchpad.net:443/**

We will likely want to remove launchpad code hosting from here as discussed in the standup this morning. Only recipes from the store should pass when the fetch service is in strict mode.

But I'm OK with keeping this if it makes testing easier, and removing afterwards.

> +{%- elif stage_name == "qastaging" %}
> +- https://git.pkg.store:443/**
> +- https://git.staging.snapcraftcontent.com:443/**
> +- https://git.qastaging.paddev.net:443/**
> +{%- endmacro %}
> +
> +{%- macro inspectors_crafts_configuration() %}
> +{%- if stage_name == "production" %}
> +- https://git.pkg.store:443/**
> +- https://git.staging.snapcraftcontent.com:443/**
> +- https://git.launchpad.net:443/**
> +{%- elif stage_name == "qastaging" %}
> +- https://git.pkg.store:443/**
> +- https://git.staging.snapcraftcontent.com:443/**
> +- https://git.qastaging.paddev.net:443/**
> +{%- endmacro %}
> +
>  series: jammy
>  applications:
>    fetch-service:
>      charm: ch:fetch-service
>      channel: edge
> -    revision: 9
> +    revision: 18
>      num_units: 1
>      expose: true
>      options:
> +        {#- Inspectors configuration for the fetch service. #}
> +        inspectors: "
> +          git:
> +            urls:
> +              {{ inspectors_git_configuration() }}
> +
> +          crafts:
> +            urls:
> +              {{ inspectors_crafts_configuration() }}
> +
> +          snap:
> +            snap-declaration:
> +              - name: publisher-id
> +                value: [canonical]
> +
> +          apt:
> +            repositories:
> +              default:
> +                urls:
> +                  - http://archive.ubuntu.com/ubuntu
> +                  - http://*.archive.ubuntu.com/ubuntu
> +                  - http://security.ubuntu.com/ubuntu
> +                  - http://ports.ubuntu.com/ubuntu-ports
> +                  - https://esm.ubuntu.com:443/ubuntu
> +                  - http://ftpmaster.internal/ubuntu
> +                dists:
> +                  - "*"
> +                components:
> +                  - "*"
> +                public-key: |
> +                  -----BEGIN PGP PUBLIC KEY BLOCK-----

I know this is a private repo, but should this be here?
It's a public key, perhaps it's fine to have it here - just want to be sure we are conscious about it if so.

Overall I'd rather we moved into having our mojo specs in the public repo - even if this isn't the case now, we shouldn't make it difficult in the future.

> +
> +                  mQINBFufwdoBEADv/Gxytx/LcSXYuM0MwKojbBye81s0G1nEx+lz6VAUpIUZnbkq
> +                  dXBHC+dwrGS/CeeLuAjPRLU8AoxE/jjvZVp8xFGEWHYdklqXGZ/gJfP5d3fIUBtZ
> +                  HZEJl8B8m9pMHf/AQQdsC+YzizSG5t5Mhnotw044LXtdEEkx2t6Jz0OGrh+5Ioxq
> +                  X7pZiq6Cv19BohaUioKMdp7ES6RYfN7ol6HSLFlrMXtVfh/ijpN9j3ZhVGVeRC8k
> +                  KHQsJ5PkIbmvxBiUh7SJmfZUx0IQhNMaDHXfdZAGNtnhzzNReb1FqNLSVkrS/Pns
> +                  AQzMhG1BDm2VOSF64jebKXffFqM5LXRQTeqTLsjUbbrqR6s/GCO8UF7jfUj6I7ta
> +                  LygmsHO/JD4jpKRC0gbpUBfaiJyLvuepx3kWoqL3sN0LhlMI80+fA7GTvoOx4tpq
> +                  VlzlE6TajYu+jfW3QpOFS5ewEMdL26hzxsZg/geZvTbArcP+OsJKRmhv4kNo6Ayd
> +                  yHQ/3ZV/f3X9mT3/SPLbJaumkgp3Yzd6t5PeBu+ZQk/mN5WNNuaihNEV7llb1Zhv
> +                  Y0Fxu9BVd/BNl0rzuxp3rIinB2TX2SCg7wE5xXkwXuQ/2eTDE0v0HlGntkuZjGow
> +                  DZkxHZQSxZVOzdZCRVaX/WEFLpKa2AQpw5RJrQ4oZ/OfifXyJzP27o03wQARAQAB
> +                  tEJVYnVudHUgQXJjaGl2ZSBBdXRvbWF0aWMgU2lnbmluZyBLZXkgKDIwMTgpIDxm
> +                  dHBtYXN0ZXJAdWJ1bnR1LmNvbT6JAjgEEwEKACIFAlufwdoCGwMGCwkIBwMCBhUI
> +                  AgkKCwQWAgMBAh4BAheAAAoJEIcZINGZG8k8LHMQAKS2cnxz/5WaoCOWArf5g6UH
> +                  beOCgc5DBm0hCuFDZWWv427aGei3CPuLw0DGLCXZdyc5dqE8mvjMlOmmAKKlj1uG
> +                  g3TYCbQWjWPeMnBPZbkFgkZoXJ7/6CB7bWRht1sHzpt1LTZ+SYDwOwJ68QRp7DRa
> +                  Zl9Y6QiUbeuhq2DUcTofVbBxbhrckN4ZteLvm+/nG9m/ciopc66LwRdkxqfJ32Cy
> +                  q+1TS5VaIJDG7DWziG+Kbu6qCDM4QNlg3LH7p14CrRxAbc4lvohRgsV4eQqsIcdF
> +                  kuVY5HPPj2K8TqpY6STe8Gh0aprG1RV8ZKay3KSMpnyV1fAKn4fM9byiLzQAovC0
> +                  LZ9MMMsrAS/45AvC3IEKSShjLFn1X1dRCiO6/7jmZEoZtAp53hkf8SMBsi78hVNr
> +                  BumZwfIdBA1v22+LY4xQK8q4XCoRcA9G+pvzU9YVW7cRnDZZGl0uwOw7z9PkQBF5
> +                  KFKjWDz4fCk+K6+YtGpovGKekGBb8I7EA6UpvPgqA/QdI0t1IBP0N06RQcs1fUaA
> +                  QEtz6DGy5zkRhR4pGSZn+dFET7PdAjEK84y7BdY4t+U1jcSIvBj0F2B7LwRL7xGp
> +                  SpIKi/ekAXLs117bvFHaCvmUYN7JVp1GMmVFxhIdx6CFm3fxG8QjNb5tere/YqK+
> +                  uOgcXny1UlwtCUzlrSaP
> +                  =9AdM
> +                  -----END PGP PUBLIC KEY BLOCK-----
> +        "
>          {#- Channel configuration for the fetch service snap. #}
>          channel: {{ fetch_service_snap_channel }}
>          log_hosts_allow: "launchpad-bastion-ps5.internal"


-- 
https://code.launchpad.net/~pelpsi/launchpad-mojo-specs/+git/private/+merge/475344
Your team Launchpad code reviewers is subscribed to branch ~launchpad/launchpad-mojo-specs/+git/private:master.