Configuration Problem Testing CIHelper

[webstech]

CIHelper expects to be in the gitgitgadget repo so it can reference the res/Welcome.MD file. When testing, the workdir for CIHelper needs to point to different locations so the tests can run in parallel and the git config can be set outside of the tester's personal config.

In CIHelper, when creating a GitGitGadget, it is done with GitGitGadget.get("."). The get method will query the config for gitgitgadget.workDir using the passed '.' directory (IOW, the git config for gitgitgadget).

1. This seems awkward because the CIHelper workdir is supposed to point to a git repository. Why not just pass that to GitGitGadget.get()?
2. The get method is using the passed directory to obtain git config info. Is this supposed to be part of the git repo config or the gitgitgadget repo config? The get method does support two directories being passed.
3. Concurrent testing has problems with different tests setting gitgitgadget.workDir. This can be worked around by changing directories to a different git config but that also requires copying res/Welcome.MD.

Hopefully this makes some sense. One solution is to provide CIHelper with a git repo (as is done now), and a location to get the config info (default to '.').

Questions? Other suggestions? Have I misunderstood everything?

[dscho]

@webstech I think you are right on all accounts. To allow for easy deployment, I would still like to default to querying gitgitgadget.workDir in ., but you've correctly pointed out that it would make a lot of sense to allow overriding that with a pair of Git/GitGitGadget workDirs (we often need the Git workDir to look at commits and notes, and we do need the GitGitGadget workDir for the welcome message).

[webstech]

Thanks for the quick reply. I know you have been very busy.

For minimal change, the CIHelper would be
constructor(workDir?: string, skipUpdate?: boolean, gggDir = "." )
and getting the gadget would be
GitGitGadget.get(this.gggDir, this.workDir).

The oddity is the reversed order from the constructor to the get method but they are different objects.

I do have a question related to deployment. How is the git configuration set up in azure? Is it part of the vm setup? Just trying to understand how this works to deploy on fresh vm.

[dscho]

For minimal change, the CIHelper would be
constructor(workDir?: string, skipUpdate?: boolean, gggDir = "." )
and getting the gadget would be
GitGitGadget.get(this.gggDir, this.workDir).

The oddity is the reversed order from the constructor to the get method but they are different objects.

Makes sense. It might be a good idea to consolidate the order?

I do have a question related to deployment. How is the git configuration set up in azure? Is it part of the vm setup? Just trying to understand how this works to deploy on fresh vm.

The pipeline has 4 steps:

• Use Node 10.x
• Pull GitGitGadget, run npm, essentially:

  test -d .git || git init || exit 1
  
  git rev-parse HEAD >.git/PRE_FETCH_HEAD &&
  git fetch https://github.com/gitgitgadget/gitgitgadget master ||
  exit 1
  
  [... commented-out dirty tricks to use PRs' patches early ...]
  git reset --hard FETCH_HEAD ||
  exit 1
  
  # If nothing was pulled, we already built it
  test "$(git rev-parse HEAD)" != "$(cat .git/PRE_FETCH_HEAD)" ||
  exit 0
  
  npm install &&
  npm run build ||
  exit 1

• Obtain GitHub Token:

  GIT_CONFIG_PARAMETERS="$GIT_CONFIG_PARAMETERS $EXTRA_CONFIG"
  node build/script/misc-helper.js set-app-token &&
  git config gitgitgadget.publishRemote \
      https://x-access-token:$(git config gitgitgadget.githubToken)@github.com/gitgitgadget/git

  (The EXTRA_CONFIG is necessary because it contains a value that is configured via a secret pipeline variable.)
• Handle PR (Comment or Push):

  GIT_CONFIG_PARAMETERS="$GIT_CONFIG_PARAMETERS $EXTRA_CONFIG"
  set -x
  if test "$(pr.comment.id)" -lt 0
  then
      case "$BUILD_SOURCEBRANCH" in
      refs/pull/[1-9]*/head|refs/pull/[1-9]*/merge)
          branchname=${BUILD_SOURCEBRANCH#refs/pull/}
          prnumber=${branchname%/*}
          ;;
      *) echo "Invalid source branch: $BUILD_SOURCEBRANCH">&2; exit 1;;
      esac
      node build/script/misc-helper.js handle-pr-push "$prnumber"
  else
      node build/script/misc-helper.js handle-pr-comment "$(pr.comment.id)"
  fi

  (The pr.comment.id pipeline variable is set when queuing the build via the Azure Function that is registered as a webhook, that's how we can discern between this mode vs push.)

The pipeline variable GIT_CONFIG_PARAMETERS (which is pre-set as an environment variable in the scripts, interpreted by Git in the same way as git -c <key>=<value> would be) is defined as 'user.name=GitGitGadget' 'user.email=gitgitgadget@gmail.com' 'gitgitgadget.workDir=$(Agent.HomeDirectory)/../git-worktree', i.e. it configures GitGitGadget as committer and it asks GitGitGadget to fetch the commits and notes to the directorygit-worktree/ that is located next to the Azure Pipeline build agent's own directory, which assumes that the agent is running in a suitable VM and its files are installed into the home directory of the account running the agent.

Ideally, this definition (even if it is very small compared to other pipeline definitions I maintain) would be tracked in a Git repository, but since we want this to be a CI build (so that it neatly shows those Checks in the PR page), the pipeline is already associated with gitgitgadget/git (even if the pipeline is configured not to "sync the source", i.e. it does not check out the code pushed to the PR), and we cannot simply add GitGitGadget's Azure Pipelines' definitions to master because that is mirrored verbatim from git/git.

[dscho]

[...] 'gitgitgadget.workDir=$(Agent.HomeDirectory)/../git-worktree', i.e. it configures GitGitGadget as committer and it asks GitGitGadget to fetch the commits and notes to the directorygit-worktree/ [...]

I should note that this is the reason why GitGitGadget initializes the worktree if it does not yet exist, so that we could easily initialize a new agent in a new VM if the current one ever gets broken or unavailable for whatever reason.

[webstech]

For minimal change, the CIHelper would be
constructor(workDir?: string, skipUpdate?: boolean, gggDir = "." )
and getting the gadget would be
GitGitGadget.get(this.gggDir, this.workDir).
The oddity is the reversed order from the constructor to the get method but they are different objects.

Makes sense. It might be a good idea to consolidate the order?

Do you feel strongly about changing the order? Changing the order for GitGitGadget.get() makes the most sense but it would mean making the workDir a required parm. The use case for overriding the default gggDir for CIHelper is only for testing.

[webstech]

Thanks for all the background info. Should this be added to a gitgitgadget wiki page or a .md file in the repo? It really helps fill in some of the blanks that are not visible when looking at the jobs on azure.

[dscho]

Do you feel strongly about changing the order?

No ;-)

Thanks for all the background info. Should this be added to a gitgitgadget wiki page or a .md file in the repo? It really helps fill in some of the blanks that are not visible when looking at the jobs on azure.

I guess a Wiki page would work fine? Or, if you want to have it closer to the code, an .md would be fine with me, too... Whatever you like.

[dscho]

Should this be added to a gitgitgadget wiki page

Done: https://github.com/gitgitgadget/gitgitgadget/wiki/GitGitGadget%27s-Azure-Function-and-Azure-Pipelines

[webstech]

@dscho Thank you for creating the wiki page. It really helps to know the internal plumbing that is not visible in the jobs.

[dscho]

Note to self: I still need to document two more pipelines: the one watching pu and the one watching public-inbox.org/git/'s master.

[dscho]

I still need to document two more pipelines: the one watching pu and the one watching public-inbox.org/git/'s master.

Done: https://github.com/gitgitgadget/gitgitgadget/wiki/GitGitGadget's-Azure-Function-and-Azure-Pipelines/_compare/af3e755f594d7cd5ffcbf836df126e5baa8d56ee...365adcd2b002bd0c937540b47821055114ce6ae1

[dscho]

@webstech thanks for reminding me of this issue when talking about #156: yes, I think that PR will address at least this part of this ticket:

3. Concurrent testing has problems with different tests setting gitgitgadget.workDir. This can be worked around by changing directories to a different git config but that also requires copying res/Welcome.MD.