-
Notifications
You must be signed in to change notification settings - Fork 51
createToken.py: Fix typo output_filename => input_filename #661
Conversation
Hey there @landgraf, first of all there are two things you need to note. For contributing to an eclipse project like kuksa you need to sign an ECA. For steps on how to do this and why see https://adoptium.net/en-GB/docs/eca-sign-off/ . Once you have done this the second issue is that we now check for the right license headers so I will suggest in another review comment what you would need to change. In the past we have not enforced this check and now we check for new changes if it is there so for that reason it must be done but is not related to your changes :) If you have any further questions please feel free to reply here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# All rights reserved. This configuration file is provided to you under the
# terms and conditions of the Eclipse Distribution License v1.0 which
# accompanies this distribution, and is available at
# http://www.eclipse.org/org/documents/edl-v10.php
change it to:
########################################################################
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
#
# SPDX-License-Identifier: Apache-2.0
########################################################################
For an example have a look at https://github.com/eclipse/kuksa.val/blob/3b27221b4b883a07374575b701fec93f8653fd4b/kuksa-client/kuksa_client/__main__.py
output_filename = input_filename[:-5] + ".token" | ||
else: | ||
output_filename = output_filename + ".token" | ||
output_filename = input_filename[:-5] if input_filename.endswith(".json") else input_filename |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no objection to the bug correction itself, the code looks nice and great that you found and addressed it @landgraf.
But both the old and new implementation does not match the example tokens that we actually have in this repository. Like we have single-read.json
and single-read.json.token
, i.e. it seems that .token
has been added and .json
has not been removed. I sort of would prefer that this script matches the example tokens we have, i.e. either just add .token
regardless of whether input is single-read.json
or single-read
, or change name of the example token. The latter would how ever require changes in all places currently using the tokens.
So maybe best to just have only
output_filename = input_filename + ".token"
i.e. do not care if input file ends with .json
or not. The reason why I prefer this option rather than renaming the tokens is that the tokens in this directory are used by KUKSA Server (C++ only, i.e. not Databroker), and we currently do not want to spend too much effort on KUKSA.val Server
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@erikbosch
I'm not sure If I got your comment.
Current code with suggested change works exactly like you've mentioned.
>>> input_filename = "single-read.jsob"
>>> input_filename = "single-read.json"
>>> output_filename = input_filename[:-5] if input_filename.endswith(".json") else input_filename
>>> output_filename += ".token"
>>> print(output_filename)
single-read.token
>>> input_filename = "single-read"
>>> output_filename = input_filename[:-5] if input_filename.endswith(".json") else input_filename
>>> output_filename += ".token"
>>> print(output_filename)
single-read.token
.json suffix deleted if exists and .token suffix added for all tokens
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've modified code a bit to make it even simpler and clever. Please re-review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but if you look at current files we have for example:
- https://github.com/eclipse/kuksa.val/blob/master/kuksa_certificates/jwt/single-read.json
- https://github.com/eclipse/kuksa.val/blob/master/kuksa_certificates/jwt/single-read.json.token
i.e. for existing tokens we did not remove the .json
suffix when generating tokens, the input is called single-read.json
and the token is called single-read.json.token
, but that is not consistent with a script that generates a single-read.token
if it gets single-read.json
as input.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not how current code works.
Prior the change:
if input_filename.endswith(".json"):
output_filename = input_filename[:-5] + ".token"
If file ends with ".json" then ".json" suffix is dropped and output_filename will be ".token" (without json).
In other words suggested change doesn't change behavior of the positive branch but fixes negative (else) branch which was broken before..
else:
output_filename = output_filename + ".token"
^ This code is broken and fails with:
NameError: name 'output_filename' is not defined
So either your example was not generated with the current version of the script or there's bug somewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I share your conclusion, the current example token pairs in the repository that all have the form <something>.json
and <something>.json.token
does not match how the script work. If you use <something>.json
as input you get <something>.token
as output. Which gives us three options:
- Keep the mismatch. Not optimal as I see it.
- Adapt the script so it matches the pattern used in the examples, i.e. never remove
.json
- Rename the examples so they match how the scripts works. If this is done we would need to do a search/replace for all places using the current names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the current example token pairs in the repository that all have the form .json and .json.token does not match how the script work.
This is not correct.
All the access tokens in the top level jwt
folder are named like this (created using this script). They all use the new authorization scopes, and have been placed there in order to not be confused with the legacy tokens found here.
Would it help (cause less confusion) to move this script as well to that folder?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad, I only thought about the folder https://github.com/eclipse/kuksa.val/tree/master/kuksa_certificates/jwt, not that https://github.com/eclipse/kuksa.val/tree/master/jwt also use this script.
Yes, we could move the script, but if so we need update *.md files referencing the script. But I think that we anyhow would benefit from having a small README.md
in this folder that explains how to regenerate the tokens if needed, which will be need if anyone want to use them after 2025-12-31.
As I propose a small README I can create a separate PR myself, as this PR itself does not change behavior for input files names <something>.json
anyway.
output_filename is not defined in else branch. Most likely author wanted to use input_filename instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
License added looks good! Codewise
401414b
to
3f12f9e
Compare
output_filename is not defined in else branch. Most likely author wanted to use input_filename instead