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

Add safety check when publishing unstable APIs #630

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rydrman
Copy link
Collaborator

@rydrman rydrman commented Jan 23, 2023

This feels a little unnecessary without the additional definition of the new API type, but I'm leaving that part in #598 so that this can be merged separately.

@rydrman rydrman added this to the V1 Spec milestone Jan 23, 2023
@rydrman rydrman self-assigned this Jan 23, 2023
@codecov
Copy link

codecov bot commented Jan 23, 2023

Codecov Report

Attention: 12 lines in your changes are missing coverage. Please review.

Comparison is base (e8cf549) 53.55% compared to head (9c0c829) 53.40%.
Report is 277 commits behind head on main.

❗ Current head 9c0c829 differs from pull request most recent head a6a69cb. Consider uploading reports for the commit a6a69cb to get more accurate results

Files Patch % Lines
crates/spk-cli/common/src/publish.rs 18.18% 9 Missing ⚠️
crates/spk-schema/src/spec.rs 66.66% 2 Missing ⚠️
crates/spk-cli/group2/src/cmd_publish.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #630      +/-   ##
==========================================
- Coverage   53.55%   53.40%   -0.16%     
==========================================
  Files         258      257       -1     
  Lines       20466    20352     -114     
==========================================
- Hits        10960    10868      -92     
+ Misses       9506     9484      -22     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rydrman rydrman mentioned this pull request Jan 23, 2023
11 tasks
@@ -33,6 +33,10 @@ pub struct Publish {
#[clap(long, short)]
force: bool,

/// Allow publishing packages with unstable api versions
Copy link
Collaborator

Choose a reason for hiding this comment

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

This help string is likely going to be misunderstood to be something related to package API compatibility / compat values or whatever.

I suggest at least expanding the description to "package spec version" or similar and it would help to briefly touch on what the dangers are for enabling this or at least mention that enabling it might be dangerous.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done, let me know what you think. Github is being slow to update so in case it doesn't, I changed it to:

    /// Allow publishing package specs written in unstable api versions
    ///
    /// The api version of a spec is the value of the `api` field within it.
    #[clap(long)]
    allow_unstable_api: bool,

}

impl RecipeApiVersion {
pub fn is_stable(&self) -> bool {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Presumably some future build of spk will have a newer recipe API version flipped to "stable" but that new build may not be in common use yet or fully deployed.

What versions are allowed to be published to a repo could be a property of the repo rather than the spk client. For example, if I configured our production "origin" repo to not accept "v1" packages until we have fully deployed v1-aware builds of spk everywhere and have retired non-v1-aware builds, then I would know that nobody would be able to publish packages to origin (accidentally or otherwise) that might break tools in production.

I'm reminded how simple things like spk ls stopped working when we were in a transition period between python and rust, because ls would crap out the first chance it got if it saw a package it didn't like.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Our goal, I believe, is still to ignore packages in a repo that are invalid. That being said, I think this is definitely an idea to explore, but if it's the same to you I'd like to start with this as it's a lot less involved. We can then circle back when the v1 is actually being discussed and when we start to re-open conversations on migration

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we last talked about this, an spk conf file got added. I think it would go a long way to address my concern if we could have this "is_stable" method return something defined the conf file, if present, otherwise it can default to v0.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The goal of is_stable was specifically for things like v1beta where the version itself is not considered stable by definition. Perhaps we need a separate concept for versions that are considered acceptable to publish that can be configured?

@rydrman
Copy link
Collaborator Author

rydrman commented Feb 14, 2024

From the meeting today:

  • @rydrman to rebase and ping again
  • let's get this in, knowing that we will want to build on it when we get there - likely being able to query some kind of allowlist within the repo itself rather than it being part of the build

Signed-off-by: Ryan Bottriell <ryan@bottriell.ca>
@rydrman rydrman requested review from jrray and removed request for jrray February 16, 2024 03:55
@rydrman
Copy link
Collaborator Author

rydrman commented Feb 16, 2024

Pinging this again for a review

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.

2 participants