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

yamf Is Overwriting Existing Manifest Entries #4

Open
CodeGat opened this issue Oct 21, 2024 · 2 comments
Open

yamf Is Overwriting Existing Manifest Entries #4

CodeGat opened this issue Oct 21, 2024 · 2 comments
Assignees

Comments

@CodeGat
Copy link
Contributor

CodeGat commented Oct 21, 2024

Background

See

declare -A manifest_paths
for path in ${{ steps.copy.outputs.paths }}; do
echo "Path is $path"
manifest_dir=$(dirname $path)
cd $manifest_dir || exit
manifest_entry_filename=$(basename $path)
echo "Generating a manifest entry in $manifest_dir for $manifest_entry_filename"
yamf add -n ${{ env.MANIFEST_FILE_NAME }} --force $manifest_entry_filename
manifest_paths["$manifest_dir/${{ env.MANIFEST_FILE_NAME }}"]=1
done
echo "${!manifest_paths[@]}" > ${{ env.REMOTE_MANIFEST_FILE_LIST_PATH }}
EOT

We are currently doing (essentially) a series yamf add -n manifest.yaml --force file1 then yamf add -n manifest.yaml --force file2 etc... calls which actually overwrites the manifest.yaml each time (as in, the file will eventually only have file2), which is not intended behavior.

But if we remove the --force, it keeps all the files in the one manifest (good), but doesn't update the manifest if we update (say) file1 (bad). A minimal example follows:

echo "hi" >> some.txt
yamf add -n manifest.yaml some.txt
cat manifest.yaml
echo "hello" >> some.txt
yamf add -n manifest.yaml some.txt
cat manifest.yaml
# Both invocations of `cat` will be the same

Solutions

  • Rework how we are doing the loop of paths so we can do something like yamf add -n manifest.yaml --force file1 file2 ...
  • Fix the yamf code so that --force isn't a full overwrite, or non---force doesn't update anything. Maybe somewhere in the middle? --update?
  • Something else?
@CodeGat CodeGat self-assigned this Oct 21, 2024
@CodeGat
Copy link
Contributor Author

CodeGat commented Oct 21, 2024

Advocating for a new yamf add --update file option that would update a manifest files file entry while keeping the other entries intact :D @aidanheerdegen

@aidanheerdegen
Copy link
Member

Advocating for a new yamf add --update file option that would update a manifest files file entry while keeping the other entries intact :D @aidanheerdegen

Yeah, or just re-work the logic here

https://github.com/ACCESS-NRI/yamanifest/blob/7f9aaaddc2d31ebe1cd1b9d92ab2df349e35ba82/yamanifest/yamf.py#L58-L62

It's just wrong. I think the check at line 60 should just be removed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants