phetsims / perennial

Maintenance tools that won't change with different versions of chipper checked out
MIT License
2 stars 5 forks source link

Is this how Maintenance.checkBranchStatus works? #309

Closed zepumph closed 1 year ago

zepumph commented 1 year ago

@samreid and @matthew-blackman and I were working on a maintenance release, and after pulling all release branches and running Maintenance.checkBranchStatus(), we had a log that looked kinda like:

. . . .
states-of-matter 1.2
  [INFO] Potential changes (dependency is not previous commit)
  [INFO] da663e80b553643694da2d4038806a1c99e29d3b e126bf3a862af3e4d32b27f076c95d6913fc4654 033ebc28c9e36f8ecac81a58246c07f5cf323f94
states-of-matter-basics 1.2
  [INFO] Potential changes (dependency is not previous commit)
  [INFO] 94a084025eb7da4597edd1536e30ca1d6db8c23b f5c88b4f5df640e2b4c676287c0c94b5e4314f1a 1691e65cd9002d757b176539d8b2eae1544d9062
trig-tour 1.0
  [INFO] Potential changes (dependency is not previous commit)
  [INFO] 73865d374b28ec68d5370f340f8bc6ab466779f5 464d72c1267e87b462871ee7e190b8819bc201fb ca843ed56edbd97bdf48828478c34944a4aca7e5
under-pressure 1.1
  [INFO] Potential changes (dependency is not previous commit)
  [INFO] 327575f3d933cc30d0a7680b4721ae9da20a51fa db0e4ffff12eabadcaf12b576958ea23559d1915 8c36fa3ca45b71c8f2c900dde039285aeb557004
unit-rates 1.0
  [INFO] Potential changes (dependency is not previous commit)
  [INFO] 5831145dec5d3a888850abab425bd08e7a640646 2336471167e9221c74082cc15396590011c8cf7e 3133d001616e712f529287c6b5ae5241af40018e
vector-addition 1.0
  [INFO] Potential changes (dependency is not previous commit)
  [INFO] 819eeeb2366e6799bc2df236eb7f79646bdaf4dc 07e89a534a38f842b1f7a0de8b5e9ed536224871 57b3e715952af38fcd0e5848eae3692006ed3fe7
vector-addition-equations 1.0
  [INFO] Potential changes (dependency is not previous commit)
  [INFO] a9b130ea519ecbd81a60d229b5c52e27bb614845 6c12f892ee38170521b0262b63e2b289b1f9ca9f 5c7572d2779aef06bd181b0afaaa5aa0f3d1ba93
wave-interference 2.0
  [INFO] Potential changes (dependency is not previous commit)
  [INFO] e2f341985c820735381fb660e60253c0617c33f5 dca551e6204f4ea604ffdfd5ec1934820b576046 bc7cc09855c854ac06a32890f5671a28a4bcf51e
wave-on-a-string 1.1
  [INFO] Potential changes (dependency is not previous commit)
  [INFO] cc3748249eb76864632a2c57789fe8aaf9661eae 1cb0b360ef999347e43497b713405a0dd13dee78 d90c12c4916d0e4749b196d615d48e2b7fd81046
waves-intro 1.1
  [INFO] Potential changes (dependency is not previous commit)
  [INFO] a3453280c657680abd72556ada44f5f8bfaa1cab 3aafd886e6890db1b79af877efe36c86240e4bd5 e722e9a7c812517bcf338920ffbef08f06dcda2a

We were confused, and so we investigated, and think we understand that this is because each tip of a release branch is out of date with its dependencies.json because of the commit sha that actually changed the dependencies.json. We updated doc, will you please have a look?

samreid commented 1 year ago

I feel there is a problem in this code:

    async getStatus( getBranchMapAsyncCallback = getBranchMap ) {
      const results = [];

      const dependencies = await this.getDependencies();
      const dependencyNames = Object.keys( dependencies ).filter( key => {
        return key !== 'comment' && key !== this.repo;
      } );

      // Check our own dependency
      if ( dependencies[ this.repo ] ) {
        try {
          const currentCommit = await gitRevParse( this.repo, 'HEAD' );
          const previousCommit = await gitRevParse( this.repo, 'HEAD^' );
          if ( dependencies[ this.repo ].sha !== previousCommit ) {
            results.push( '[INFO] Potential changes (dependency is not previous commit)' );
            results.push( `[INFO] ${currentCommit} ${previousCommit} ${dependencies[ this.repo ].sha}` );
          }

The report from @zepumph's log above is showing currentCommit and previousCommit from master, not from the branch. That's why it is saying there is a discrepancy. So maybe we need to handle the main sim repo differently?

jonathanolson commented 1 year ago

Fixed a big I created with the above patch. @samreid can you confirm this is fixed?

samreid commented 1 year ago

Looks fixed, thanks! Do you want to revert or revise @zepumph commit above, which is https://github.com/phetsims/perennial/commit/2166859d92aa78f80c3ba62e12ac6d0d4ea433e0

jonathanolson commented 1 year ago

Reverted above. All good here?

zepumph commented 1 year ago

Perfect! And I also confirmed that it is fixed after the commit. Thanks.