Skip to content

Conversation

Gladear
Copy link

@Gladear Gladear commented Jun 26, 2023

Attempt to fix #26

It's pretty hard to test if it actually works since I don't have multiple nodes to test, so sorry if the fix isn't correct 😕

Copy link
Owner

@tompave tompave left a comment

Choose a reason for hiding this comment

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

Apologies for the long wait, and thank you for giving this a shot.

I'm not sure if this solution is the right one, though.
Did it work locally? Unfortunaly CI can't run on this PR because it's based on an older commit of the master branch where the CI image was too old. If you could merge the latest master in and push, CI will be able to run.

@Gladear Gladear force-pushed the fix/allow-deleting-non-atom-flag branch from 1fab278 to 1360c34 Compare July 25, 2023 08:41
@Gladear Gladear requested a review from tompave July 25, 2023 08:44
@tompave
Copy link
Owner

tompave commented Jul 25, 2023

Hi @Gladear , thank you for iterating on this.
I'd ask to please not commit --amend and force push. It blows away the PR history, and it makes it difficult to see what changed. You can simply add a new incremental commit and push.

@tompave
Copy link
Owner

tompave commented Jul 25, 2023

Ok, I think this works in theory, thank you.

However, I don't think that the error from #26 comes only from the delete "/flags/:name/boolean" endpoint you've modified. As I mentioned here, the problem is more widespread. I think that all endpoints need to switch to logic that ensures that String.to_existing_atom/1 doens't just rely on what's available in memory.

From the original message:

It's pretty hard to test if it actually works since I don't have multiple nodes to test, so sorry if the fix isn't correct

On the master branch of FWF I've added some instructions on how to run multiple nodes: https://github.com/tompave/fun_with_flags#working-with-pubsub-locally

But I don't think you need that. You can restart the FWF.UI app (iex -S mix) and go directly to a flag details page without first loading the homepage.

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.

ArgumentError :erlang.binary_to_existing_atom/2
2 participants