pkgjs / wiby Goto Github PK

I've been reviewing #3, and I think for now it only deals with public GH. This would be a handy tool for private GH instances too.

Happy to PR once #3 / #4 are in.

The README is currently empty. We need to list information such as

• Available commands
• Token required

Hi Team!

I'd like disable successComment option on semantic-release bot in order to prevent it from commenting under each released PR/issue.

WDYT?

Simplify the abstracting of a GitHub owner and repo from the Git url in the package.json by using `git-url-parse'.

Scenarios:

1. Suppose you're on a branch some-branch on the parent repo
2. You run wiby test
3. One of the dependent repos already has a branch called wiby-some-branch
   a) It could be that you already ran wiby test and did not wiby clean afterwards (most common)
   b) It could be that whoever was testing, wanted to make additional changes on the branch to get the tests to pass with the linked parent
   c) It could be that it exists, because multiple dependencies are being tested

Thinking out loud:

• In cases (b) and (c) we probably need to link the parent into package.json and push a commit into an existing branch; if the parent is already linked, then we probably want to push an empty commit (git commit -m "This is a blank commit" --allow-empty) to re-trigger the builds.
• In case (a), we probably want to delete the branch, recreate it from HEAD and link the parent again.
  • The problem here is that we don't know if we're dealing with (a) or (b)/(c) - in case of (b)/(c) we want to keep all changes
  • The simple approach would be to not rebase, however that may mean that wiby test is running against outdated code (i.e. it no longer answers the question "will I break you"...)
  • Maybe the right answer is to always attempt to update the existing branch with master, before pushing a new commit? Can that be done via the API? We could then notify the user about the conflicts, if any, via wiby results?

Related issues:

• #54
• #78

Originally raised point from #14:

Changes the getPatch function name to getCommitURL

The above, to me, is the "version of the parent (?) package expressed as an npm dependency descriptor", which is probably a horrible name for a function, however the "version of the parent package" is something we'll be referring to quite a bit, so may as well find a short word for that?

But we probably also have other terms (such as the "parent" package itself), that we may want to define so that we can use them consistently across the code and documentation.

At the moment, wiby test will link the sha of the master into the dependents.

It should instead link the branch that it's running against. Possibly try to detect the branch using env-ci if it's running on a detached head.

I'll fix this together with #78 - just documenting the issue.

At the moment wiby result just dumps some json - we can do better than that (although --json would still be useful!)

At the moment, when a wiby test label is added, it will kick off the workflow, which will run wiby test if there is no status check for wiby; if there is a pending status check, it will instead run a wiby result and report the status.

There currently is no way to clean up the status check, therefore there is no way to re-start the wiby test if for some reason the tests got stuck and the results are never moving past "pending".

Ideas:

• add support for a wiby clean test label, which would clear the status check, run a wiby clean and only then kick off a wiby test
  • gotcha: as wiby clean closes pending PRs / deletes pending branches, it might destroy any user changes in these branches
• add support for a wiby retest label, which would clear the status check and re-run wiby test
  • gotcha: we probably then also need an explicit wiby clean label?
• regardless of anything, we should probably remove all the wiby * trigger labels at the end of any workflow run

While we don't need to focus on this right now, I'm opening up this discussion early, so that people can share their thoughts and things they've seen in the wild.

To implement #6, there will be a need to have at least two tokens - one for public GH, one for GH Enterprise, which means that just the GITHUB_TOKEN environment variable will not be enough.

Does anyone have any preferred solutions for that?

I personally have either used or have seen the following in the wild:

• Have a GITHUB_COM_TOKEN and have a secondary env var token. Renovate takes this approach (they use a RENOVATE_TOKEN and GITHUB_COM_TOKEN).
  • I'm personally a bit torn about this approach. I think it is critical to be able to provide all secrets via environment variables, because that's just a more common approach in different ways these things get run (inside containers, in k8s clusters, directly on various CI platforms). However on the other hand, the logic and the naming can become confusing. Also having just the two tokens may be kind of limiting (while it's probably very rare that you'd need more than one Github Enterprise, there's talk at a client right now that we might be moving from a per-project GHE to a globally shared GHE, meaning there'll likely be an overlap period and a migration). This can be solved by having a config file mapping out which token belongs to which URL?
• "Borrow" credentials from other apps, if available. E.g. on macOS you might have git osx keychain credentials helper installed, or you could be using hub, which has credentials stored in ~/.config/hub.
  • I'm personally a bit torn about this approach as well (a recurring theme, alright). On the one hand, the right thing to do is to have separate credentials per app, and the various credentials already present were not set up for our purpose, so it's kind of shady to do that, on the other hand - it's damn nice UX. If you're on a computer, and the credentials are already available and saved, why not use them (after asking for permission once)?
• Use octokit-auth-netrc
  • In a way, this is a variation of above, but if I get it right - it was also built as a generic store standard? I do kind of like this approach, although it is likely not enough, because depending on the security requirements of where wiby will be running, it might be very inconvenient operationally to provide a single file with all the credentials. I kind of wish .netrc could point to an env var as well as provide the secret value itself...
• All of the above?

As per #3 (comment)

I know someone mentioned this earlier at some point, but either way - I went ahead and created https://github.com/wiby-test and invited the same people who are contributors/owners here. I recommend you unsubscribe from notifications there :)

I figure a separate org is better for this, to avoid polluting pkgjs with repositories which are only used for automated tests. Don't think we need any kind of policies around these repositories - the intent is to just close everything that happens there?

I plan to next scaffold out some test repos there, which will always produce the same test results, so that we can use them to test wiby end-to-end (there's already some repos being accessed in the unit tests, but they're under @andrewhughes101's account, so I'll replace those).

Ref: https://github.com/pkgjs/wiby/blob/master/docs/DRAFT-FLOW.md step 5

For each module in the wiby.json, retrieve their package.json and determine whether the module is likely to automatically be updated to the new version of your module once it is released.

This could be implemented as a check which needs to be overridden (i.e. only test dependents which would match the current range) or it could just be an option we output as part of results.

Ref: https://github.com/pkgjs/wiby/blob/master/docs/DRAFT-FLOW.md step 9

Open a draft PR in the dependents repository that contains the patched package.json.

Depending on the CI setup, it might not be enough to just push the branch - wiby.json should allow setting a configuration option to additionally open a PR.

This also means that we'd probably need to close the PR, etc, when the tests are done.

• Support the --file argument
• Reads a wiby.json (initially based on the draft structure)

OK, I'll be straight up - I can't handle standard anymore 😂 It's slowing me down.

My preferred style is https://hapi.dev/policies/styleguide/ (the key being loooots of whitespace - 4 spaces, extra line at the start of the function, no no-multiple-empty-lines, etc) - unless there's objections, I'd like to switch to that?

Ref: https://github.com/pkgjs/wiby/blob/master/docs/DRAFT-FLOW.md step 3, step 10

wiby will look for an existing log file for this HEAD, and error if it finds one. This will stop wiby test being run multiple times. You can override this check with wiby test --overwrite.

We might want to record the test references, e.g. which repos actually got kicked off, which commit status to check, etc, as at the moment it just relies on a matching branch in dependent repos.

Some options where to put that data:

• a log file, as suggested initially
  • only available locally, but maybe that's good enough?
• commit status
  • unsure how much metadata we can store there
  • a variation would be to have a status per dependent, but unsure if there are any limits on how many status checks can a commit have
• git notes
  • might be trickier to implement and would need some dancing around
• just stick to a convention and re-scan all the dependents based on wiby.json each time

After a PR is closed/merged, we should automatically run wiby clean for that branch.

This implies support for wiby clean --branch [specific name] (at the moment, wiby clean works off the currently active parent branch), as the branch to be cleaned up will be missing.

Bonus points: support wiby clean comment to trigger the cleanup as well.

I'm more familiar with Travis - happy to PR that.

Current test suites do not cover CLI flow. As we see here (#25 (comment)) breaking yargs was not reflected in tests - all was green.

Will adding tests for CLI interface be valuable at current project phase?

Hi Team,

I noticed that tests on main are red - https://github.com/pkgjs/wiby/actions?query=branch%3Amain

Just wanted to clarify is it known issue or I can work on the fix?

Right now we use the repository URL from package.json to determine the git repository of the parent (dependency) package, and we link the dependents to use a branch from this repository

This is also used to check out a branch based on the branch name of the PR in the workflow.

This behavior is incorrect and fails.

When wiby is running in CI triggered from a PR which based off a fork, we need to:

• keep using the branch name of the fork (except when it is main, see bullet point in #78) for constructing the testing branch name (e.g. wiby-my-feature)
• reference the PR based branch name from the target (i.e. parent) repo (e.g. pkgjs/wiby#pull/ID/head)
• use git fetch origin pull/ID/head:BRANCHNAME approach to check out the branch inside the action

When wiby is running locally, we need to:

• keep using the branch name of the fork
• reference the branch name and repository of the origin
  • detect if the branch has an upstream set? see also: last bullet point in #46

At the moment wiby will check that the parent module is one of dependencies - but it could as well be in devDependencies or even peerDependencies.

Per the discussion in today's meeting, wanted to create an issue for checking out the repository_dispatch trigger for an Action, which allows an Action to be triggered by a GitHub API call. This might be useful for triggering an Action in the root Wiby repo to report back when a run fails/succeeds.

At the moment, wiby test creates a branch called wiby-[dep name] in the dependent repos. There is nothing to clean this up - running a new wiby test will fail if the branch already exists.

wiby test should not fail on existing branch - but that is a separate, larger problem (I'll create a separate issue), but in the meantime - deleting the wiby-wiby branch manually on wiby-test/* repos is a hassle during development.

wiby clean should take the following options:

• --all - remove all wiby-* branches (by default only remove the current branch, based on the naming as proposed in #78)
• --dry-run - show the branches that would be removed
  • alternative: do the dry-run by default and ask the user to confirm the action? With an optional --force to skip the confirmation?
  • bonus points: what if the branches contain commits other than ones made by wiby?

Ref: https://github.com/pkgjs/wiby/blob/master/docs/DRAFT-FLOW.md step 3

Support wiby test --commit=[sha] (if left out - keep current behavior and test currently checked out HEAD)

• Something to kick off wiby test (triggered off a comment?)
• Something to poll for results and report them back as a comment

could we co-ordinate some time in the near future to switch the default branch name?

If we can use https://octokit.github.io/rest.js/v18#repos-create-or-update-file-contents to push the updated package.json to the dependent repository we can reduce the need for separate api calls to create a blob, tree, commit and branch

I had not been contributing to Wiby as I had other commitments during 2020 and so I came a fresh perspective ( meaning I did not really understand it at first).

What I think is needed is refresh of the readme so that it is more obvious what wiby does for a prospective user and how that is achieved.

I know it is early days at the moment but the readme file is the window into the tool and is equally as important as the code itself.

What we need to highlight is

• Wiby's purpose and why you would want to use it
• How it works, equally important so that folks want to use it
• Many examples

There is an error with this repository's Renovate configuration that needs to be fixed. As a precaution, Renovate will stop PRs until it is resolved.

Error type: Cannot find preset's package (github>whitesource/merge-confidence:beta)

wiby needs to be able to run against forked repositories (as it might not be desirable to spam the original repositories of dependents with test branches/PRs, let alone the person testing might not have the necessary permissions to do so). When running against forked repositories, wiby should automatically keep them up to date with upstream.

First mentioned in #14 (review):

And it will be great to add standard command run to test script

While I wouldn't call standard "great" per se , we might as well add it to the npm test, unless there's any objections or we plan to change the style?

• #95 test should verify the branches do not already exist before pushing
• result should not crash on branches which are not found
• test should verify (locally available) commit that's being tested is available on GH (draft flow, step 2)

I want to use this. When can I use this? 😄

We all know about CITGM, but are you aware of https://www.npmjs.com/package/fugl ?

Not sure what you want to do with this, but at minimum, it may be helpful in design decisions

At the moment, wiby test will create a branch called wiby-[dependency name] on the dependent being tested (and wiby result will check here).

This is very restricting - it means that you can only test one set of changes in the dependency at a time (i.e. if you have multiple PRs open on wiby, they would both try to write into wiby-wiby). Moreover, this does not allow automated linking of multiple dependencies into a single test branch.

The proposal here is to name the branch based on the dependency branch, i.e. if I'm testing feat/new-stuff on wiby, then the test branch would be called wiby-feat/new-stuff on the dependent. This way other dependencies could have feat/new-stuff and would all link into the same wiby-feat/new-stuff branch.

• The wiby- prefix is necessary, to avoid touching non-wiby branches that people may be working on. It also makes it easier to implement the wiby clean...
• If the dependency branch already starts with wiby- - the prefix will not get added multiple times. Rationale: people may need to make tweaks to make the tests pass, esp. when linking multiple dependencies together.
• If the dependent already has a branch called feat/new-stuff, then wiby should base the wiby-feat/new-stuff off that, rather than off main / master.
• Bonus points: figure out how to best name the wiby-* branch when the branch being tested is a master / main on a fork of the dependency. wiby-author-master? wiby-pr-NNN [if PR already exists - it might not in the pure CLI case]?

Examples

Assuming the following imaginary dependency tree:

• update-notifier
  • is-ci
    • ci-info
  • ci-info

Someone is authoring things in ci-info and wants to make sure they don't break is-ci and update-notifier.

New feature in ci-info

1. The author has push access to ci-info, they create a new branch with feat/awesome-stuff
2. They wiby test
3. wiby creates a wiby-feat/awesome-stuff branch in is-ci and in update-notifier where it updates the ci-info to be pulled from git #feat/awesome-stuff.

New feature spanning ci-info and is-ci

1. The author has push access to ci-info, they create a new branch with feat/awesome-stuff
2. The author has push access to is-ci, they create a new branch with feat/awesome-stuff
3. They wiby test in ci-info
4. wiby creates a wiby-feat/awesome-stuff branch in is-ci (based off feat/awesome-stuff) and in update-notifier where it updates the ci-info to be pulled from git #feat/awesome-stuff.
5. They wiby test in is-ci
6. wiby updates wiby-feat/awesome-stuff in update-notifier to pull is-ci from #wiby-feat/awesome-stuff
   • This means that update-notifier now has ci-info linked from git, and deduped as well.

New feature in ci-info, wiby uncovers tweaks required in is-ci

1. The author has push access to ci-info, they create a new branch with feat/awesome-stuff
2. They wiby test
3. is-ci breaks - the author pushes some modifications to fix that directly into wiby-feat/awesome-stuff branch
4. They wiby test in is-ci
5. wiby sees wiby-feat/awesome-stuff branch in update-notifier and links is-ci from git #wiby-feat/awesome-stuff branch.

(I'll consider drawing up some diagrams for this...)

This issue provides visibility into Renovate updates and their statuses. Learn more

Awaiting Schedule

These updates are awaiting their schedule. Click on a checkbox to get an update now.

• chore(deps): lock file maintenance

• Check this box to trigger a request for Renovate to run again on this repository

Ref: nodejs/package-maintenance#413

Should also set the engines field.

Ref: https://github.com/pkgjs/wiby/blob/master/docs/DRAFT-FLOW.md step 1

I'd suggest we use renovate, mostly cause I'm familiar with it.