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

[CERTTF-424] feat: support a reference directory for attachments #374

Merged
merged 9 commits into from
Nov 5, 2024

Conversation

boukeas
Copy link
Contributor

@boukeas boukeas commented Oct 9, 2024

Description

At the moment, when Testflinger attachments are not specified through absolute path names (in the local field) they are interpreted relevant to the current working directory. This is quite restrictive and becomes an inconvenience especially when Testflinger jobs and their attachments are created in Github workflows and submitted through the Testflinger submit Github action.

This PR:

  • Changes the default behaviour to interpret relative attachment path names against the directory where the submitted job file is located.
  • Introduces the --attachments-relative-to command-line argument to the Testflinger CLI, for specifying the directory that relative attachment path names will be resolved against.
  • Introduces a new input to the Testflinger submit Github action, corresponding to the new command-line argument
  • Adds documentation for the additional CLI and Github action arguments

Resolved issues

Resolves CERTTF-424.

Tests

  • Manual tests in a local Testflinger deployment
  • New unit tests have been introduced to test attachment packing, including packing when a reference directory is provided.

Changes to the Github submit action can only be tested once the updated Testflinger CLI snap has been deployed.

@boukeas boukeas requested review from plars and Hook25 October 9, 2024 16:53
@boukeas boukeas marked this pull request as ready for review October 9, 2024 16:53
plars
plars previously approved these changes Oct 11, 2024
Copy link
Contributor

@plars plars left a comment

Choose a reason for hiding this comment

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

This looks fine to me, and I'm +1 on it but just one question...
Was attachments-relative-to-job-file specifically requested in addition to attachments-relative-to? It feels a bit overkill to have both. I mean... I get it, but I also feel like it's not that hard for the submitter to specify the dir where the attachments are if they are also already specifying the same dir in the path to the job file. Curious what your thoughts are about that and how critical it seemed to have both? I know you talked to Max about though, so perhaps there's something that I'm not considering.

@boukeas
Copy link
Contributor Author

boukeas commented Oct 14, 2024

This looks fine to me, and I'm +1 on it but just one question... Was attachments-relative-to-job-file specifically requested in addition to attachments-relative-to? It feels a bit overkill to have both. I mean... I get it, but I also feel like it's not that hard for the submitter to specify the dir where the attachments are if they are also already specifying the same dir in the path to the job file. Curious what your thoughts are about that and how critical it seemed to have both? I know you talked to Max about though, so perhaps there's something that I'm not considering.

The request by @Hook25 was specifically for something like attachments-relative-to-job-file. I simply wanted to generalise that since the request is essentially a special case of the more general feature that attachments-relative-to offers.

I wouldn't say it's critical to have both but it feels like a shame to specify two arguments (the reference path and the job file) that would be almost identical if the job file is in the reference path.

@Hook25
Copy link
Contributor

Hook25 commented Oct 14, 2024

I don't think its critical to have both but setting them in CI is not super handy (quite the opposite actually) so to have a simple boolean it is way better for me. Also, I suggested this as the attachment mechansim attaching from the cwd is counterintuitive to me. I expected it (and I still think it should) be relative to the job file by default, as that is what most tools do when they get an input recipe

@boukeas
Copy link
Contributor Author

boukeas commented Oct 14, 2024

I expected it (and I still think it should) be relative to the job file by default, as that is what most tools do when they get an input recipe

I think that's a fair point that I hadn't considered appropriately before. Indeed, examples like HTML or LaTeX files interpret relative paths as relative to the location of the input file. @plars would you object to removing the --attachments-relative-to-job-file flag altogether and making this behaviour the default? The current default, i.e. using the current working directory as a reference, would be easily selected using the --attachments-relative-to argument.

@plars
Copy link
Contributor

plars commented Oct 17, 2024

I don't think its critical to have both but setting them in CI is not super handy (quite the opposite actually) so to have a simple boolean it is way better for me.

I was kind of assuming it would be something like:

JOB_PATH=/root/of/jobs/and/attachments
testflinger submit $JOB_PATH/job.yaml --attachments-relative-to $JOB_PATH

I suspect you're right that in practice, attachments will probably be in the same location as the job file most of the time. However I'm reluctant to change it from defaulting to CWD because:

  1. It's a change from the current behavior
  2. It's a change from expected behavior of most CLI programs (i.e. when you don't give a path, CWD is assumed)

If something like the above snippet doesn't work for just keeping --attachments-relative-to as the primary mechanism to do this, then I'm fine with just landing this as-is.

@boukeas
Copy link
Contributor Author

boukeas commented Oct 18, 2024

It's a change from expected behavior of most CLI programs (i.e. when you don't give a path, CWD is assumed)

@plars that's exactly how I approached it when I made the initial choice to use the CWD but I believe I was wrong (and that's the reason why I am insisting we should revert that choice and change the default behaviour). I believe this is not an issue of where a CLI program manipulates its own files, in which case using the CWD would indeed be the convention. This is a case of how relative links in the input file are interpreted. The best example of a similar example I can think of is HTML. Regardless of the browser you are using or the CWD, relative references in an HTML file are always interpreted in relation to the HTML file. And that makes a lot of sense for a myriad reasons there is no point in repeating. We need the additional flexibility here, i.e. we need the --attachments-relative-to argument, but I really think the default should be what is now the --attachments-relative-to-job-file behaviour.

…b file

Which makes the `--attachments-relative-to-job-file` argument redundant
@boukeas boukeas force-pushed the CERTTF-424-attachment-reference-directory branch from aedefa4 to 771efb9 Compare October 30, 2024 12:45
@boukeas
Copy link
Contributor Author

boukeas commented Oct 30, 2024

This has stagnated for a while so I thought I'd take the executive decision to:

  • remove the --attachments-relative-to-job-file argument
  • make that behaviour the default, i.e. relative path attachments are interpreted in relation to the job file, in line with similar approaches to interpreting relative paths in e.g. HTML files.
    I have also asked for some feedback on MM around this feature and received another response in favour of this approach.

@boukeas boukeas requested a review from plars October 30, 2024 13:27
Copy link
Contributor

@plars plars left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for the updates to the docs and the tests too! If "Mr Hook25" (@Hook25) is good with it, then let's merge it! :)

@boukeas boukeas merged commit 5226580 into main Nov 5, 2024
5 checks passed
@boukeas boukeas deleted the CERTTF-424-attachment-reference-directory branch November 5, 2024 19:42
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.

3 participants