Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat/add flavor page #389

Merged
merged 15 commits into from
Dec 6, 2023
Merged

Feat/add flavor page #389

merged 15 commits into from
Dec 6, 2023

Conversation

garloff
Copy link
Member

@garloff garloff commented Nov 25, 2023

Code for PoC for SovereignCloudStack/issues#483

Totally Web 1.0, that's my generation ...

@garloff garloff requested review from berendt and mbuechse November 25, 2023 22:58
Signed-off-by: Kurt Garloff <[email protected]>
@berendt
Copy link
Contributor

berendt commented Nov 26, 2023

Works for me. I already added it to the flavor manager docs:

https://osism.github.io/docs/guides/operations-guide/openstack/flavor-manager/#name-parser-and-generator

Copy link
Contributor

@mbuechse mbuechse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! From my POV, two things should be improved though:

  • the script should output valid HTML; to that end, it could read the index.html and insert its output into some conveniently marked (easy to find) position
  • the error messages should also be improved; all I got at first try was TypeError: cannot unpack non-iterable NoneType object -- it could at least say "The cause is probably that the name given is not a valid SCS flavor name"

As always, I volunteer to implement these or similar improvements.

@garloff
Copy link
Member Author

garloff commented Nov 27, 2023

Nice! From my POV, two things should be improved though:

* the script should output valid HTML; to that end, it could read the index.html and insert its output into some conveniently marked (easy to find) position

I had this on my list as well.
I would actually generate the main page from the script and embed the flavor parser (and later flavor generator) output in it. Maybe using some simple templating with zope or so would make this easy.
I also need to replace the cgi module with urllib.

* the error messages should also be improved; all I got at first try was `TypeError: cannot unpack non-iterable NoneType object` -- it could at least say "The cause is probably that the name given is not a valid SCS flavor name"

For some wrong flavors you do get useful hints where the parser failed, for some you don't.
My guess is that there are a few things that you can easily improve in the existing implementation, but I would be surprised if you are able to output meaningful error messages in all cases without adding a lot of error handling code.

An option is of course to use some open source tooling that is meant to support syntax parsing rather than coding it yourself. I had a quick check back then and decided that coming up with my own is easier. But that may have been due to insufficient research.

If you go there, we'd have a second independent implementation of the flavor spec, which may help with testing.

As always, I volunteer to implement these or similar improvements.

I would want to take the easy steps (using templating to generate good HTML) quickly but then do a first merge.
The more tedious error message improvements can always come in a second step.
Oh, and of course if someone wants to use less antique Web1.0 technology ...

@garloff
Copy link
Member Author

garloff commented Dec 3, 2023

So, this should be producing good HTML now.
It also outputs "non-SCS flavor" for flavors not starting with SCS-.
Next step will be to support flavor name generation, but that needs a somewhat complex form, which I can't do in a few minutes ... so I'd like this one to be merged. Reviews welcome!

@berendt
Copy link
Contributor

berendt commented Dec 4, 2023

I think we can merge this AS-IS in a first step. This can be improved and beautified in a 2nd step. Same with the generator.

# from a CGI form with flavor
#
# (c) Kurt Garloff <[email protected]>, 11/2023
# SPDX-License-Identifier: CC-BY-SA-4.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be AGPLv3 for code?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All code in this directory is CC-BY-SA-4.0, which is a weak copyleft license.

CC is not a good license for compiled code, as it does not have any provisions for source code availability when distributing binary code. That is not a problem for scripting languages ...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if one could distribute pyc files. I guess it would probably be a hassle for everyone.

Copy link
Contributor

@mbuechse mbuechse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall fine, only very minor things in the comments.

Plus: could you add some snippet documenting how you integrated the cgi script into the website? Some config for nginx or Apache?

@@ -271,6 +271,9 @@ It goes beyond the above example in checking that the discoverable
features of flavors (vCPUs, RAM, Disk) match what the flavor names claim.
This is used for SCS-compatible compliance testing.

The web page <https://flavors.scs.community/>
has a flavor name parser and (soon) generator.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should add that the functionality of the website is analogous to some of the scripts mentioned above (and in what way).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"The flavor name parsing and (soon) flavor name generation of these scripts is also exposed via the web page https://flavors.scs.community/."

# import os
import sys
import re
# import traceback
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we get rid of the imports that are commented out?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those were required before but have been replaced ...
Feel free to push a change to remove them.

@garloff garloff merged commit 42bfb4f into main Dec 6, 2023
@garloff garloff deleted the feat/add-flavor-page branch December 6, 2023 05:31
@garloff garloff mentioned this pull request Dec 8, 2023
@joshmue
Copy link
Contributor

joshmue commented Jan 22, 2024

For future reference: There was a injection bug here that's fixed in #403 and #404.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants