CompositionalIT / farmer

Repeatable Azure deployments with ARM templates - made easy!
https://compositionalit.github.io/farmer
MIT License
523 stars 157 forks source link

Add ability to associate metadata with Storage Account Queues. Metada… #981

Closed cmarshall10450 closed 1 year ago

cmarshall10450 commented 1 year ago

…ta can be added for a single queue, multiple queues with unique metadata or multiple queues with the same metadata

This PR closes #916

The changes in this PR are as follows:

I have read the contributing guidelines and have completed the following:

If I haven't completed any of the tasks above, I include the reasons why here: The unit tests don't pass but I could not work out why. I cannot see a difference in the actual and expected values.

Below is a minimal example configuration that includes the new features, which can be used to deploy to Azure:

let storageAccount = storageAccount {
    name = "testaccount"
    add_queue_with_metadata "myqueue" (Map [
        "source", "imageStore"
    ])
}
martinbryant commented 1 year ago

@cmarshall10450 Hi I know this request has been open for a while and would you possibly have time to re-visit this?

cmarshall10450 commented 1 year ago

Hi @martinbryant

I'll try to take a look at this over the weekend. Is there a consensus on how the DSL should look. I like @ninjarobot's builder proposal. What do you think?

martinbryant commented 1 year ago

Hi @cmarshall10450 and thanks for replying.

I would go with @ninjarobot builder suggestion as I think the goal here is to make the DSL clearer.

cmarshall10450 commented 1 year ago

@martinbryant, I agree. This project is my only experience of F#, so I'll need to get back into the mindset of F# development first.

cmarshall10450 commented 1 year ago

@ninjarobot / @martinbryant Hopefully the changes that I have made are in line with what was suggested. I'm not familiar with F# so there will definitely be things to be improved on. I had some issues with getting the tests to pass - expected is same as actual but fails.

martinbryant commented 1 year ago

@cmarshall10450 thank you for your changes - hopefully you have seen that the builds fail due to fantomas formatting required - @theprash @isaacabraham could you please have a look at reviewing the changes please

cmarshall10450 commented 1 year ago

Thanks @martinbryant. I've fixed the formatting issues.

ninjarobot commented 1 year ago

@cmarshall10450 after doing a release notes update, tests were failing when merged with the updates in master due to a newer Expecto which improved sequence comparison logic. I fixed those tests - can you please review the fixes?

ninjarobot commented 1 year ago

Not sure why GitHub isn't updating the PR with updates to the branch. I'll merge and reapply the formatting fixes.

cmarshall10450 commented 1 year ago

@ninjarobot All looks good to me. Thanks for fixing the tests.