Skip to content

Delete invalid symlinks referencing identity-idp-config#8786

Merged
mitchellhenke merged 1 commit intomainfrom
mitchellhenke/prevent-broken-static-asset-sync
Jul 17, 2023
Merged

Delete invalid symlinks referencing identity-idp-config#8786
mitchellhenke merged 1 commit intomainfrom
mitchellhenke/prevent-broken-static-asset-sync

Conversation

@mitchellhenke
Copy link
Contributor

@mitchellhenke mitchellhenke commented Jul 14, 2023

🛠 Summary of changes

This is still a bit experimental and I'm working to confirm that the theory below is correct and the proposed solution resolves the problem we're seeing every now and then.

We've run into the situation a couple times where a built application artifact includes symlinks to the identity-idp-config directory. Since identity-idp-config is cloned each time regardless of whether the artifact exists for the SHA, it is possible for an SP logo to be deleted from identity-idp-config which breaks the symlink and can cause build or provisioning issues.

This PR goes through the target public directory for SP logos and deletes any symlinks that exist but do not link to an existing file.

Other options:

  • Delete all symlinks regardless of whether they are valid since we'll be re-creating them
    • This is quite straightforward and would be my preferred alternative
  • Do not include SP logo symlinks in the build artifact
    • This is a bit more work on the platform side and is likely more complicated to implement and maintain since it would require changes outside of this repo

changelog: Bug Fixes, Asset Compilation, Delete invalid symlinks referencing identity-idp-config
@mitchellhenke mitchellhenke requested review from a team, mmagsa and zachmargolis July 14, 2023 21:13
@mitchellhenke mitchellhenke marked this pull request as draft July 14, 2023 21:13
Dir.entries(idp_logos_dir).each do |name|
next if name.start_with?('.')
target = File.join(idp_logos_dir, name)
File.rm(target) if File.symlink?(target) && !File.file?(target)
Copy link
Contributor

Choose a reason for hiding this comment

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

would we ever have to worry that we have a symlink to a directory or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not currently I don't think. The logo directory should only contain files.

Copy link
Contributor

@Sgtpluck Sgtpluck left a comment

Choose a reason for hiding this comment

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

i like the approach! if it turns out it doesn't work for some reason, would the delete all/recreate symlinks approach still be in play?

is there value in testing against zach's comment? (ie, that there are only files in the directory)

@mitchellhenke mitchellhenke marked this pull request as ready for review July 17, 2023 15:29
@mitchellhenke
Copy link
Contributor Author

i like the approach! if it turns out it doesn't work for some reason, would the delete all/recreate symlinks approach still be in play?

Yep, absolutely

is there value in testing against zach's comment? (ie, that there are only files in the directory)

I lean towards excluding it for now since I'm not sure the implementation can accommodate anything but a single level of files. My assumption from there is if that changed, we could tackle both at the same time?

@mitchellhenke mitchellhenke merged commit 049a180 into main Jul 17, 2023
@mitchellhenke mitchellhenke deleted the mitchellhenke/prevent-broken-static-asset-sync branch July 17, 2023 17:22
@aduth aduth mentioned this pull request Jul 19, 2023
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