diff mbox series

[v2,YOCTO,#15882] Add question mark to mark the start of the AZ_SAS query parameters.

Message ID 20250529141418.1939172-1-robbin.vandamme@renson.be
State New
Headers show
Series [v2,YOCTO,#15882] Add question mark to mark the start of the AZ_SAS query parameters. | expand

Commit Message

robbin van damme May 29, 2025, 2:14 p.m. UTC
Signed-off-by: Robbin Van Damme <robbinvandamme@gmail.com>
---
 lib/bb/fetch2/az.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Gyorgy Sarvari May 29, 2025, 3:34 p.m. UTC | #1
On 5/29/25 16:14, robbin van damme via lists.openembedded.org wrote:
> Signed-off-by: Robbin Van Damme <robbinvandamme@gmail.com>
> ---
>  lib/bb/fetch2/az.py | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/lib/bb/fetch2/az.py b/lib/bb/fetch2/az.py
> index 346124a8b..327273379 100644
> --- a/lib/bb/fetch2/az.py
> +++ b/lib/bb/fetch2/az.py
> @@ -36,7 +36,7 @@ class Az(Wget):
>  
>          az_sas = d.getVar('AZ_SAS')
>          if az_sas and az_sas not in ud.url:
> -            ud.url += az_sas
> +            ud.url += f'?{az_sas}'
>  
>          return Wget.checkstatus(self, fetch, ud, d, try_again)
>  
> @@ -62,7 +62,7 @@ class Az(Wget):
>          az_sas = d.getVar('AZ_SAS')
>  
>          if az_sas:
> -            azuri = '%s%s%s%s' % ('https://', ud.host, ud.path, az_sas)
> +            azuri = '%s%s%s?%s' % ('https://', ud.host, ud.path, az_sas)
>          else:
>              azuri = '%s%s%s' % ('https://', ud.host, ud.path)
>  
I think that this is going to be a documentation problem.
Though not mentioned in the commit message unfortunately, I believe I
see the issue: the url has the classic
"protocol://domain/path?param=value&param2=value2..." format. The AZ_SAS
variable holds the "param=value..." part. The original commit message[1]
does contain a (not too strong) reference that it should start with a
question mark, however this didn't reach the documentation, and it gives
an example without a question mark[2].

I think the documentation should be updated primarily. Of course the
code could be a bit smarter too, for example if az_sas doesn't start
with '?', then maybe emit a debug message and prepend a '?'
automatically. Though I don't know how many people rely on this fetcher,
but I don't think the current behavior should be changed with suddenly
not expecting a '?' in this variable.

[1]:
https://git.openembedded.org/bitbake/commit/lib/bb/fetch2/az.py?id=b103b02f2ce2f8f5079f17ec1a854f904c2110a4
[2]:
https://docs.yoctoproject.org/bitbake/2.6/bitbake-user-manual/bitbake-user-manual-fetching.html#az-fetcher-az
robbin van damme May 30, 2025, 7:21 a.m. UTC | #2
Hi,

Ok thats fine.
Lets forget this patch then.
I submitted other one with update of the docs on how to use the AZ_SAS.

kind regards,

Robbin


On Thu, May 29, 2025 at 5:34 PM Gyorgy Sarvari <skandigraun@gmail.com>
wrote:

> On 5/29/25 16:14, robbin van damme via lists.openembedded.org wrote:
> > Signed-off-by: Robbin Van Damme <robbinvandamme@gmail.com>
> > ---
> >  lib/bb/fetch2/az.py | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/bb/fetch2/az.py b/lib/bb/fetch2/az.py
> > index 346124a8b..327273379 100644
> > --- a/lib/bb/fetch2/az.py
> > +++ b/lib/bb/fetch2/az.py
> > @@ -36,7 +36,7 @@ class Az(Wget):
> >
> >          az_sas = d.getVar('AZ_SAS')
> >          if az_sas and az_sas not in ud.url:
> > -            ud.url += az_sas
> > +            ud.url += f'?{az_sas}'
> >
> >          return Wget.checkstatus(self, fetch, ud, d, try_again)
> >
> > @@ -62,7 +62,7 @@ class Az(Wget):
> >          az_sas = d.getVar('AZ_SAS')
> >
> >          if az_sas:
> > -            azuri = '%s%s%s%s' % ('https://', ud.host, ud.path, az_sas)
> > +            azuri = '%s%s%s?%s' % ('https://', ud.host, ud.path,
> az_sas)
> >          else:
> >              azuri = '%s%s%s' % ('https://', ud.host, ud.path)
> >
> I think that this is going to be a documentation problem.
> Though not mentioned in the commit message unfortunately, I believe I
> see the issue: the url has the classic
> "protocol://domain/path?param=value&param2=value2..." format. The AZ_SAS
> variable holds the "param=value..." part. The original commit message[1]
> does contain a (not too strong) reference that it should start with a
> question mark, however this didn't reach the documentation, and it gives
> an example without a question mark[2].
>
> I think the documentation should be updated primarily. Of course the
> code could be a bit smarter too, for example if az_sas doesn't start
> with '?', then maybe emit a debug message and prepend a '?'
> automatically. Though I don't know how many people rely on this fetcher,
> but I don't think the current behavior should be changed with suddenly
> not expecting a '?' in this variable.
>
> [1]:
>
> https://git.openembedded.org/bitbake/commit/lib/bb/fetch2/az.py?id=b103b02f2ce2f8f5079f17ec1a854f904c2110a4
> [2]:
>
> https://docs.yoctoproject.org/bitbake/2.6/bitbake-user-manual/bitbake-user-manual-fetching.html#az-fetcher-az
>
>
Richard Purdie May 30, 2025, 7:50 a.m. UTC | #3
On Fri, 2025-05-30 at 09:21 +0200, robbin van damme via
lists.openembedded.org wrote:
> Hi,
> 
> Ok thats fine.
> Lets forget this patch then.
> I submitted other one with update of the docs on how to use the
> AZ_SAS.

What might also help is adding a sanity check to the fetcher to ensure
the variable does start with "?".

If it does not, we could raise an error so the user knows what is
wrong.

Cheers,

Richard
diff mbox series

Patch

diff --git a/lib/bb/fetch2/az.py b/lib/bb/fetch2/az.py
index 346124a8b..327273379 100644
--- a/lib/bb/fetch2/az.py
+++ b/lib/bb/fetch2/az.py
@@ -36,7 +36,7 @@  class Az(Wget):
 
         az_sas = d.getVar('AZ_SAS')
         if az_sas and az_sas not in ud.url:
-            ud.url += az_sas
+            ud.url += f'?{az_sas}'
 
         return Wget.checkstatus(self, fetch, ud, d, try_again)
 
@@ -62,7 +62,7 @@  class Az(Wget):
         az_sas = d.getVar('AZ_SAS')
 
         if az_sas:
-            azuri = '%s%s%s%s' % ('https://', ud.host, ud.path, az_sas)
+            azuri = '%s%s%s?%s' % ('https://', ud.host, ud.path, az_sas)
         else:
             azuri = '%s%s%s' % ('https://', ud.host, ud.path)