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

Extend implementation notes #643

Merged
merged 8 commits into from
Sep 5, 2024
Merged

Conversation

cah-hbaum
Copy link
Contributor

Add and extend some implementation notes derived from the findings of #426.

@cah-hbaum cah-hbaum added enhancement New feature or request standards Issues / ADR / pull requests relevant for standardization & certification SCS is standardized SCS is standardized SCS-VP10 Related to tender lot SCS-VP10 labels Jun 19, 2024
@cah-hbaum cah-hbaum requested a review from mbuechse June 19, 2024 08:36
@cah-hbaum cah-hbaum self-assigned this Jun 19, 2024
@cah-hbaum cah-hbaum linked an issue Jun 19, 2024 that may be closed by this pull request
@cah-hbaum cah-hbaum force-pushed the 559-extend-implementation-notes branch 2 times, most recently from 5e5def6 to a076ace Compare June 19, 2024 08:54
@cah-hbaum cah-hbaum requested a review from martinmo June 19, 2024 08:54
@cah-hbaum cah-hbaum force-pushed the 559-extend-implementation-notes branch from a076ace to a2f2650 Compare June 19, 2024 09:00
@cah-hbaum
Copy link
Contributor Author

Not sure if the link checker is good in this state or how links should be entered.
I wrote in some links to a file that will be create WITH this PR. I already wrote in the main branch, since it WILL be there soon, but it isn't yet, so this gets marked as a dead link...

Which would be the correct workflow here? (maybe @martinmo, I think he worked on this)

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.

Thanks for the good work! The supplements should be a bit more succinct; they are not stand-alone documents, and they shouldn't duplicate content from the main document. They should, however, be actionable, and I think this has been achieved. Unfortunately, for one of the documents, there is a conflict with another pending (albeit approved) PR, and I have some reservations about the new spec file.

Standards/scs-0101-w1-entropy-implementation-testing.md Outdated Show resolved Hide resolved
Standards/scs-0101-w1-entropy-implementation-testing.md Outdated Show resolved Hide resolved
@@ -0,0 +1,107 @@
---
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to incur quite some maintenance work whenever we change the set of standard images. To me, it would be preferable to guide people to the specs provided by OSISM and tell them to use those parts that they deem relevant, including (of course) those that are mandatory from the POV of SCS. Maybe we can work with OSISM to make this process automatic; then we should open an issue about that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand where you come from, but we already provide a set of standard images in the form https://github.com/SovereignCloudStack/standards/blob/main/Tests/iaas/scs-0104-v1-images.yaml.
So IMO, its not that much of a deal to also provide this other file, since its practically is derived from the first one.
And we also don't say that this is the only solution, but its quite a good guidance.

Copy link
Contributor

Choose a reason for hiding this comment

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

The set of standard images you're referring to is merely fixing the image source, so people know what to expect when they encounter a certain name. The spec file, on the other hand, contains quite a lot of specific details that I (as a standards person) have absolutely no expertise about. So I'm not convinced.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with Hannes here: to make it easier for a CSP to become compliant, it would be really nice to have a complete, working YAML that can just be fed to the openstack-image-manager. At the moment, you'll need to assemble parts of it yourself (I exercised this recently):

However, I disagree with the file name choice: it should reflect that it is meant for v1 of scs-0104 and to be fed to the image manager.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, people will have to do this. However, we can't keep this file up to date, which is what OSISM (semi-) automatically does in their repo. That we used the versions wrong is just the evidence that we shouldn't meddle in this.

@cah-hbaum
Copy link
Contributor Author

Most things should be adressed with 804b5c3

@martinmo
Copy link
Member

martinmo commented Jul 17, 2024

Because Hannes is blocked, I'll adopt this PR (I need it for SovereignCloudStack/docs#212)

cah-hbaum and others added 3 commits September 5, 2024 10:27
Add and extend some implementation notes derived from the findings of #426.

Signed-off-by: Hannes Baum <[email protected]>
Adjustments made possible by the review from @mbuechse.

Signed-off-by: Hannes Baum <[email protected]>
Co-authored-by: Matthias Büchse <[email protected]>
Signed-off-by: Martin Morgenstern <[email protected]>
@mbuechse mbuechse force-pushed the 559-extend-implementation-notes branch from bf7a3e7 to bc5edea Compare September 5, 2024 08:27
@mbuechse mbuechse marked this pull request as ready for review September 5, 2024 09:35
@mbuechse mbuechse removed enhancement New feature or request standards Issues / ADR / pull requests relevant for standardization & certification SCS is standardized SCS is standardized SCS-VP10 Related to tender lot SCS-VP10 labels Sep 5, 2024
Copy link
Contributor

@artificial-intelligence artificial-intelligence left a comment

Choose a reason for hiding this comment

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

LGTM as far as I can see on mobile

@mbuechse mbuechse merged commit a389059 into main Sep 5, 2024
4 checks passed
@mbuechse mbuechse deleted the 559-extend-implementation-notes branch September 5, 2024 09:44
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.

Extend implementation notes by findings from #426
4 participants