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

Implement CI checks pipeline #23

Closed
1 of 8 tasks
f-f opened this issue May 15, 2020 · 12 comments · Fixed by #140
Closed
1 of 8 tasks

Implement CI checks pipeline #23

f-f opened this issue May 15, 2020 · 12 comments · Fixed by #140
Labels
CI help wanted Extra attention is needed
Milestone

Comments

@f-f
Copy link
Member

f-f commented May 15, 2020

There are a bunch of things that we need to enforce in CI, so that we don't add packages that don't follow the criterias we defined.

Checks we want to run for every package:

  • verify that the package name satisfies the constraints (as defined in Package name constraints #3)
  • verify that the repo value is the same for all the versions of a package (to prevent packages "stealing" package names)
  • verify that the license is a valid SPDX expression (see Implement SPDX license expressions #13)
  • verify that the repo is on GitHub (see Only accept packages from GitHub? #15)
  • reject packages with ES6 syntax in their FFI (see #24)
  • ensure that a package has a lib Target
  • ensure that a package being added has sources: ["src/**/*.purs"] in its lib Target (see here)
  • ensure that no subdir is present to new packages until we are confident that downstream tooling can deal with it (see here)

This issue is about:

  • tracking that all these checks are implemented
  • writing them down in the spec, so that clients can implement them too
@f-f f-f added CI help wanted Extra attention is needed labels May 15, 2020
@hdgarrood
Copy link
Contributor

Regarding the repo value: what do you think we should do if a package moves from one person's account to another after having had at least one version published? Could the check be that any given repo value should appear in at most one package in the repo (across all versions of each package)?

@f-f
Copy link
Member Author

f-f commented May 15, 2020

@hdgarrood I'd go for making things simple and ask to rename the package when changing the repo in the manifest.

I'm aware that many packages went through this (e.g. aff), but GitHub makes it possible without breaking links, so the url in the manifest should not change (you can see that in all the manifests the URL still points at slamdata/purescript-aff, and things work fine).
Keeping the old urls is a safe thing to do because packages are fetched from their repo URL only at publishing time (by the curator), while package managers download the tarball from the registry.

@hdgarrood
Copy link
Contributor

It's a bit unfortunate that we'll end up with lots of essentially inaccurate information that way, though, and also I don't think there's anything to stop someone else coming in and taking a repo name which previously had a redirect set up. I don't think what I'm proposing is too complicated: given a packages :: Map PackageName (Map Version Repo), you're essentially doing this:

let
  -- A map of repos "claimed" by each package
  reposClaimed :: Map PackageName (List Repo)
  reposClaimed =
    map (List.nub <<< Map.values) packages

  -- flip it around to map repos to the packages that claim them
  reposClaimedBy :: Map Repo (List PackageName)
  reposClaimedBy =
    reposClaimed
      # Map.toList

       -- gets us a List (Tuple (List Repo) PackageName)
      # map swap

       -- gets us a List (Tuple Repo PackageName)
      # List.concatMap (\(Tuple repos pkgName) -> map (\r -> Tuple r pkgName) repos)

      -- turn each tuple into a Map Repo (List PackageName)
      # map (\(Tuple repo pkgName) -> Map.singleton repo (List.singleton pkgName))

      -- combine
      # Map.unionWith (<>)
in
  for_ (Map.toList reposClaimedBy) \(Tuple repo pkgNames) -> do
    when (List.length pkgNames > 1) do
      throwError $ show repo <> " is claimed by " <> show pkgNames

@f-f
Copy link
Member Author

f-f commented May 16, 2020

@hdgarrood thanks for the example. That looks good, but the situation that I want to prevent with the check is not packages sharing a repo (which should actually happen in the case of packages published from monorepos) but some situation in which person foo comes and registers a version of the prelude under the repo foo/prelude - this wouldn't be prevented by the code above.
Note: while at first we should navigate with manual approval, I'm working under the assumption that packages should be added to the registry automatically and with no human in the loop, so I'd lean towards making these checks "safe"

@f-f f-f added this to the v1 milestone May 20, 2020
@hdgarrood
Copy link
Contributor

Oh right, I see, thanks. Did you consider storing a list of maintainers for each package? That is, a list of people who are permitted to add new versions. By default, the first person to claim a package name would become the sole maintainer, and they can subsequently add others. I think that's how most package registries work.

@milesfrain
Copy link

Could we add a check for if a major or minor bump is omitted?

For example, can we diff the package's API and check for:

  • Function added --> minor bump required
  • Function changed --> major bump required

@f-f
Copy link
Member Author

f-f commented Jul 22, 2020

@hdgarrood yeah having a list of maintainers for each package sounds like the best way to go. The check would work such that only maintainers that included in the list for the previous version would be able to add releases - this means that before a new maintainer is allowed to publish a release there should be a previous release where one of the current maintainers adds the new maintainer to the list.

@milesfrain that would be nice. How would we implement this though?

@hdgarrood
Copy link
Contributor

Is it worth being able to store certain package information in a separate place, where it's not tied to any specific version? I feel like adding or removing a maintainer ideally shouldn't require a release. I think API diffing should be implemented inside the compiler, it's going to be difficult to get right otherwise. For instance, changing the name of a type variable shouldn't count as a breaking change.

@f-f
Copy link
Member Author

f-f commented Jul 22, 2020

@hdgarrood you mean something like storing a maintainers file for every package?

@hdgarrood
Copy link
Contributor

I think we might want to make a space for storing information about a package which isn’t tied to any particular version, or maybe won’t be present when a version is first uploaded, and I don’t think the list of maintainers is the only such information. For example, I think deprecations (“$user deprecated versions X-Y of this package for $reason”) also fit into this category. Arguably so does the repository homepage.

@f-f
Copy link
Member Author

f-f commented Jul 24, 2020

Makes sense. Let's have a metadata.json file for every package then?

@hdgarrood
Copy link
Contributor

Sounds good to me

@f-f f-f modified the milestones: v1, v0.1 Sep 13, 2020
@f-f f-f closed this as completed in #140 Apr 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants